Closed Bug 1318650 Opened 3 years ago Closed 3 years ago

Link hover is very laggy with a huge places.sqlite and the Link Status Redux add-on.

Categories

(Toolkit :: Places, defect, P1)

50 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla53
Tracking Status
firefox50 --- fixed
firefox51 --- fixed
firefox52 --- verified
firefox53 --- verified

People

(Reporter: emk, Assigned: mak)

References

Details

(Keywords: perf, regression, Whiteboard: [fxsearch])

Attachments

(1 file)

I'm using the Link Status Redux add-on and my places.sqlite is larger than 350MB. When I update to Firefox 50, "Most Visited" menu becomes very slow to respond the mouse moves.

Looks like Firefox 50 makes a irreversible change to the places.sqlite. Once updated, downgrading didn't fix the issue. I had to copy an old backup to bisect the issue.

Regression range: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=fcec96be60db&tochange=81bc6dff086a1b4769398446d16f25a3f2da81a5

I suspect bug 889561 is the cause.
Yes the db change is irreversible, it actually improved perf in many instances like link coloring, but clearly it could make some less used features slower.

That db size is abnormal, I'm surprised it works at all. Is the add-on itself adding info to the db, or did you disable expiration? In normal conditions we expect places.sqlite to not be larger than 70MB, with larger dbs we cannot guarantee perf. So I'm interested into understanding why your db is so large, even if it's unrelated with this specific bug.

I checked the add-on source code, and I actually found a bug in our code.

Here we (I) forgot to update the querying to use the new index:

http://searchfox.org/mozilla-central/rev/935627b015aaabbd2dffa90cce051521f22dd7e6/toolkit/components/places/nsNavHistory.cpp#3264

I'll work on a patch and uplift it as far as possible. Good catch!
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [fxsearch]
Thanks to the quick response.

(In reply to Marco Bonardo [::mak] from comment #1)
> That db size is abnormal, I'm surprised it works at all. Is the add-on
> itself adding info to the db, or did you disable expiration? In normal
> conditions we expect places.sqlite to not be larger than 70MB, with larger
> dbs we cannot guarantee perf. So I'm interested into understanding why your
> db is so large, even if it's unrelated with this specific bug.

Yes I disable the history expiration. Firefox 49 worked pretty well despite of the huge db size.
I'd suggest to still limit history somehow, even just through my add-on: https://addons.mozilla.org/en-US/firefox/addon/expire-history-by-days/
I'm sure you could find an history limit that can satisfy your use case, maybe 2 years or such. Being completely unlimited may become a problem long term.
well, maybe we could evaluate the fix for 50.0.1, asking around.
I verified that this patch fixed the issue. Thank you!
Comment on attachment 8812549 [details]
Bug 1318650 - Searching Places views by url became extremely slow.

https://reviewboard.mozilla.org/r/94250/#review94716
Attachment #8812549 - Flags: review?(adw) → review+
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/b593a1ede43e
Searching Places views by url became extremely slow. r=adw
https://hg.mozilla.org/mozilla-central/rev/b593a1ede43e
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment on attachment 8812549 [details]
Bug 1318650 - Searching Places views by url became extremely slow.

Approval Request Comment
[Feature/regressing bug #]: bug 889561
[User impact if declined]: Any add-on doing an uri-match against history (through direct API, SDK or webext) will be much slower than usual, making the UI quite laggy and slow.
[Describe test coverage new/current, TreeHerder]: functionality is covered by existing tests, there's no places-talos perf test.
[Risks and why]: the risk is very very low, it's a oneline change just adding one condition to a SQL query. On the other side the perf benefit for affected users is quite high.
[String/UUID change made/needed]: none
Attachment #8812549 - Flags: approval-mozilla-release?
Attachment #8812549 - Flags: approval-mozilla-beta?
Attachment #8812549 - Flags: approval-mozilla-aurora?
Comment on attachment 8812549 [details]
Bug 1318650 - Searching Places views by url became extremely slow.

Fix a perf regression related to perform an uri-match against history by add-on.
Beta51+ and Aurora52+. Should be in 51 beta 3.

NI Ritu for FF50 as this is a perf regression.
Flags: needinfo?(rkothari)
Attachment #8812549 - Flags: approval-mozilla-beta?
Attachment #8812549 - Flags: approval-mozilla-beta+
Attachment #8812549 - Flags: approval-mozilla-aurora?
Attachment #8812549 - Flags: approval-mozilla-aurora+
Hello Masatoshi, could you please verify this issue is fixed as expected on a latest Nightly/Aurora build? Thanks!
Flags: needinfo?(rkothari) → needinfo?(VYV03354)
I have verified that the issue has been fixed with the latest Nightly and Developer Edition.
Status: RESOLVED → VERIFIED
Flags: needinfo?(VYV03354)
Comment on attachment 8812549 [details]
Bug 1318650 - Searching Places views by url became extremely slow.

New regression in 50, let's include the fix in 50.1.0
Attachment #8812549 - Flags: approval-mozilla-release? → approval-mozilla-release+
Version: unspecified → 50 Branch
You need to log in before you can comment on or make changes to this bug.