Closed
Bug 1318650
Opened 8 years ago
Closed 8 years ago
Link hover is very laggy with a huge places.sqlite and the Link Status Redux add-on.
Categories
(Toolkit :: Places, defect, P1)
Tracking
()
VERIFIED
FIXED
mozilla53
People
(Reporter: emk, Assigned: mak)
References
Details
(Keywords: perf, regression, Whiteboard: [fxsearch])
Attachments
(1 file)
(deleted),
text/x-review-board-request
|
adw
:
review+
gchang
:
approval-mozilla-aurora+
gchang
:
approval-mozilla-beta+
ritu
:
approval-mozilla-release+
|
Details |
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.
Assignee | ||
Comment 1•8 years ago
|
||
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]
Reporter | ||
Comment 2•8 years ago
|
||
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.
Assignee | ||
Comment 3•8 years ago
|
||
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.
Assignee | ||
Updated•8 years ago
|
status-firefox50:
--- → wontfix
status-firefox51:
--- → affected
status-firefox52:
--- → affected
status-firefox53:
--- → affected
Keywords: perf
Assignee | ||
Comment 4•8 years ago
|
||
well, maybe we could evaluate the fix for 50.0.1, asking around.
Comment hidden (mozreview-request) |
Reporter | ||
Comment 6•8 years ago
|
||
I verified that this patch fixed the issue. Thank you!
Comment 7•8 years ago
|
||
mozreview-review |
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
Comment 9•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Updated•8 years ago
|
Keywords: regression
Assignee | ||
Comment 10•8 years ago
|
||
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 11•8 years ago
|
||
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+
Comment 12•8 years ago
|
||
bugherder uplift |
Comment 13•8 years ago
|
||
bugherder uplift |
Hello Masatoshi, could you please verify this issue is fixed as expected on a latest Nightly/Aurora build? Thanks!
Flags: needinfo?(rkothari) → needinfo?(VYV03354)
Reporter | ||
Comment 15•8 years ago
|
||
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+
Updated•8 years ago
|
Version: unspecified → 50 Branch
You need to log in
before you can comment on or make changes to this bug.
Description
•