1299256 - extension with multiple locales hangs firefox for a few minutes

Status: VERIFIED FIXED

(WebExtensions :: Untriaged, defect, P1)

Description

[Shane Caraveo (:mixedpuppy)]

I have a web extension with 27 locale files, ranging from 100K to 200K each.  Installing the addon in the current beta (49b8) hangs firefox for at least a couple minutes, with 90+% cpu usage.

Comment 1

[Kris Maglione [:kmag]]

Can you attach or send me a copy of the add-on?

We do load all locales when the extension is first installed, but we only do minimal processing on them, so I'm surprised it's that noticeable of a problem.

Comment 2

[Shane Caraveo (:mixedpuppy)]

To a lesser extent, using the addon manager to disable then enable the addon also produces a noticable delay.

Comment 3

[Kris Maglione [:kmag]]

Attachment: Bug 1299256: Fix performance issues when stripping comments from large locale files.

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

Comment 4

[Kris Maglione [:kmag]]

Comment on attachment 8786450 [details]
Bug 1299256: Fix performance issues when stripping comments from large locale files.

Approval Request Comment
[Feature/regressing bug #]: Bug 1196283
[User impact if declined]: This bug causes Firefox to hang, often for over a minute, when first installing an extension containing a large amount of locale data.
[Describe test coverage new/current, TreeHerder]: These changes affect the code which strips comments from JSON files. The behavior of that code is well-tested in all affected versions, but there are no tests for this particular issue, since it's the kind of performance issue which does not lend itself well to automated testing.
[Risks and why]: Low. This patch makes minor changes to a regular expression which improves its performance, but does not change its results for any valid input. It could potentially produce different results for invalid JSON with unterminated string literals, but that input would be rejected with or without this patch.
[String/UUID change made/needed]: None.

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8786450 [details]
Bug 1299256: Fix performance issues when stripping comments from large locale files.

https://reviewboard.mozilla.org/r/75386/#review73568

Oh man... this is very subtle (and my head still hurts a bit, luckily we don't support the multiline comments, it is better if I don't even start to imagine what kind of "regular expression monster" we would use :-))

Anyway, if I read this change correctly:

we are explicitly adding the '\n' character to the chars that will invalidate a match, and this should make the processing of a big json locale file with comments faster

am I reading it right?

It would be nice to be able to have a test that ensures that we are able to load a reasonable big locale file in a reasonable amount of time over the time.

Anyway, I briefly tried locally to load some big file with the two regular expressions (before and after the fix) and it seems to confirm that this new one is faster (at least a bit faster of the previous one).

Comment 6

[Shane Caraveo (:mixedpuppy)]

Attachment: sample_locales-1.xpi

This xpi is an example of a web extension with a full set of locale files, and which causes a long/heavy hang during installation, and a noticable hang when enabling (after disabling) in the addon manager.

Comment 7

[Kris Maglione [:kmag]]

(In reply to Luca Greco [:rpl] from comment #5)
> we are explicitly adding the '\n' character to the chars that will
> invalidate a match, and this should make the processing of a big json locale
> file with comments faster

With or without comments. In fact, the performance is more of a problem in
files with more lines but fewer comments, since we currently match from the
start of each line until either the first comment, or the end of the file.

> It would be nice to be able to have a test that ensures that we are able to
> load a reasonable big locale file in a reasonable amount of time over the
> time.

It's not really doable. Timing in tests isn't reliable enough to handle
something like this.

Comment 8

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/21ec43d33e0c2ac8049355c506282ce076625578
Bug 1299256: Fix performance issues when stripping comments from large locale files. r=rpl

Comment 9

[Kris Maglione [:kmag]]

Shane, would you mind confirming this when the inbound build is done?

Comment 10

[Shane Caraveo (:mixedpuppy)]

nice!  No noticeable delay when installing.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/21ec43d33e0c

Comment 12

[Gerry Chang [:gchang]]

Comment on attachment 8786450 [details]
Bug 1299256: Fix performance issues when stripping comments from large locale files.

This patch fixes a performance issue when installing extension with large locale files. Take it in aurora and 49 beta.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d73202a4a0cd

Comment 14

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

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

Comment 15

[CosminB]

This issue is verified as fixed on Firefox 51.0a1 (2016-09-02), Firefox 50.0a2 (2016-09-02), Firefox 49.0b9 unbranded build (20160901141622) under Windows 7 64-bit, Ubuntu 16.04 32-bit and Ubuntu 12.04 64-bit.

There is no hanging when the WebExtension is installed.