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

Status: VERIFIED FIXED

(Toolkit :: Places, defect, P1)

Description

[Masatoshi Kimura [:emk]]

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.

Comment 1

[Marco Bonardo [:mak]]

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!

Comment 2

[Masatoshi Kimura [:emk]]

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.

Comment 3

[Marco Bonardo [:mak]]

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.

Comment 4

[Marco Bonardo [:mak]]

well, maybe we could evaluate the fix for 50.0.1, asking around.

Comment 5

[Marco Bonardo [:mak]]

Attachment: Bug 1318650 - Searching Places views by url became extremely slow.

Review commit: https://reviewboard.mozilla.org/r/94250/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/94250/

Comment 6

[Masatoshi Kimura [:emk]]

I verified that this patch fixed the issue. Thank you!

Comment 7

[Drew Willcoxon :adw]

Comment on attachment 8812549 [details]
Bug 1318650 - Searching Places views by url became extremely slow.

https://reviewboard.mozilla.org/r/94250/#review94716

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b593a1ede43e
Searching Places views by url became extremely slow. r=adw

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b593a1ede43e

Comment 10

[Marco Bonardo [:mak]]

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

Comment 11

[Gerry Chang [:gchang]]

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.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ff1bde52cb38

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/929e6cabdaa5

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hello Masatoshi, could you please verify this issue is fixed as expected on a latest Nightly/Aurora build? Thanks!

Comment 15

[Masatoshi Kimura [:emk]]

I have verified that the issue has been fixed with the latest Nightly and Developer Edition.

Comment 16

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-release/rev/1d9eb80bfd850f931ade03c9619b9994b82dd2f4