Open Bug 1844868 Opened 1 year ago Updated 11 months ago

4.4x slower than Chrome on the single String.prototype.split call in Editor-CodeMirror

Categories

(Core :: JavaScript Engine, defect, P2)

defect

Tracking

()

People

(Reporter: mstange, Unassigned)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [sp3])

Of the overall measured time in the Editor-CodeMirror subtest, 15% is taken up by a single call to String.prototype.split. It's splitting 29869 line string (the contents of this file) by the regular expression /\r\n?|\n/.

Reducing our time to Chrome's would win us 11% on this subtest.

A considerable portion of our time is spent in GC.

Firefox: https://share.firefox.dev/3Y3KFB1 (3715 samples)
Chrome: https://share.firefox.dev/3K9pvvw (829 samples)

We're calling RegExpMatcher from RegExpSplit and that's pretty wasteful because it allocates a full match result object each time.

It's likely common for the regexp to have no capture groups. We could check for that and then potentially make use of my patches in bug 1839422. Ideally we wouldn't have a perf cliff if capture groups are used though. I'll take a look at those internal RegExpMatcher calls.

Flags: needinfo?(jdemooij)
Depends on: 1845256

I've added an optimization for regexp-split-without-capture-groups on top of my patches in bug 1839422.

Perf results show that Editor-CodeMirror/Long/Sync improves by 29%, Editor-CodeMirror/Long/total by 19% and Editor-CodeMirror/total by 10%.

We could maybe make this a little bit faster by inlining the new intrinsic in the JITs, I'm working on a patch for that too.


Although most of the calls are with a regexp without capture groups, Speedometer 3 unfortunately also has a bunch of calls for the following monster that does have capture groups:

('(?:(?:\\')?[^'])*'|"(?:(?:\\")?[^"])*"|\s+|\btrue\b|\bfalse\b|\bintersect\b|//|==|!=|>=|<=|&&|\|\||\bin\b|\.|\[|\]|\||\{|\}|:|,|\(|\)|\?|\+|-|\*|/|%|\^|>|<|!|\b[a-zA-Z_\$][a-zA-Z0-9_\$]*\b|(?:(?:[0-9]*\.[0-9]+)|[0-9]+))

(In reply to Jan de Mooij [:jandem] from comment #3)

Although most of the calls are with a regexp without capture groups, Speedometer 3 unfortunately also has a bunch of calls for the following monster that does have capture groups:

Oh nevermind, this isn't in Speedometer 3. I think it's chrome code in toolkit/components/utils/mozjexl.js

Depends on: 1839422

This should be a lot closer to Chrome now after bug 1839422 has landed. If you have a chance to confirm this when you're back that'd be great.

Fixing bug 1846297 might also improve this test a little.

Flags: needinfo?(jdemooij) → needinfo?(mstange.moz)
Severity: -- → S4
Priority: -- → P2

(In reply to Jan de Mooij [:jandem] from comment #5)

This should be a lot closer to Chrome now after bug 1839422 has landed.

Indeed, it improved from 4.4x of Chrome to 1.5x of Chrome. Nice work!

Firefox: https://share.firefox.dev/3qHK1gi (1300 samples)
Chrome: https://share.firefox.dev/45r9l98 (867 samples)

Flags: needinfo?(mstange.moz)

(In reply to Markus Stange [:mstange] from comment #6)

Indeed, it improved from 4.4x of Chrome to 1.5x of Chrome. Nice work!

Firefox: https://share.firefox.dev/3qHK1gi (1300 samples)
Chrome: https://share.firefox.dev/45r9l98 (867 samples)

Thanks for the new profile. It looks a lot better than the one in comment 0 :) The main thing that stands out now is the nursery GC, probably because we allocate so many dependent strings for the result array.

You need to log in before you can comment on or make changes to this bug.