|Submitter||Diff||Changes||Open Issues||Last Updated|
|Error loading review requests:|
58 bytes, text/x-review-board-request
|Details | Review|
617.47 KB, application/x-xpinstall
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.
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.
To a lesser extent, using the addon manager to disable then enable the addon also produces a noticable delay.
Assignee: nobody → kmaglione+bmo
Priority: -- → P1
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 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).
Attachment #8786450 - Flags: review?(lgreco) → review+
Created attachment 8786857 [details] 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.
(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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/21ec43d33e0c2ac8049355c506282ce076625578 Bug 1299256: Fix performance issues when stripping comments from large locale files. r=rpl
Shane, would you mind confirming this when the inbound build is done?
status-firefox48: --- → affected
status-firefox49: --- → affected
status-firefox50: --- → affected
status-firefox51: --- → affected
status-firefox-esr45: --- → unaffected
nice! No noticeable delay when installing.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
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.
status-firefox49: affected → fixed
status-firefox50: affected → fixed
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.
Status: RESOLVED → VERIFIED
status-firefox49: fixed → verified
status-firefox50: fixed → verified
status-firefox51: fixed → verified
You need to log in before you can comment on or make changes to this bug.