1419785 - The RegExp constructor can spend a lot of time under CheckPatternSyntax

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Jan de Mooij [:jandem]]

This hurts us on the "Prettier" test that's part of V8's Web Tooling benchmark.

The micro-benchmark below takes 171 ms because we keep checking the pattern syntax. What we should do instead is see if we already have a RegExpShared* for this pattern/flags and then we can just use that.

That improves the micro-benchmark from 171 ms to 11 ms and it makes us about as fast as V8 on the Prettier test (25% faster than before).

function f() {
    var t = new Date;
    var s = "[\u001B\u009B][[\]()#;?]*(?:(?:(?:[a-zA-Z\d]*(?:;[a-zA-Z\d]*)*)?\u0007)|(?:(?:\d{1,4}(?:;\d{0,4})*)?[\dA-PRZcf-ntqry=><~]))";
    for (var i = 0; i < 100000; i++) {
        re = new RegExp(s);
    }
    print(new Date - t);
}
f();

Comment 1

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #0)
> What we should do instead is see if we already have a RegExpShared*
> for this pattern/flags and then we can just use that.

One wrinkle here is that the RegExpCreate intrinsic can create RegExpShareds without checking the syntax. That doesn't seem right. arai, can I change this?

Comment 2

[Tooru Fujisawa [:arai]]

(In reply to Jan de Mooij [:jandem] from comment #1)
> (In reply to Jan de Mooij [:jandem] from comment #0)
> > What we should do instead is see if we already have a RegExpShared*
> > for this pattern/flags and then we can just use that.
> 
> One wrinkle here is that the RegExpCreate intrinsic can create RegExpShareds
> without checking the syntax. That doesn't seem right. arai, can I change
> this?

Yes, it should check pattern there.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Patch

Looking up and/or allocating a RegExpShared is pretty cheap compared to parsing the regexp pattern. This patch improves the micro-benchmark in comment 0 from 171 ms to 11 ms.

The micro-benchmark below improves from 93 ms to 15 ms.

Our Octane-regexp score doesn't change.

function f() {
    var t = new Date;
    var s = "abcdefgh.*[abc\d]";
    var re = new RegExp(s);
    for (var i = 0; i < 100000; i++) {
        re1 = new RegExp(re, "u");
    }
    print(new Date - t);
}
f();

Comment 4

[Jan de Mooij [:jandem]]

I saw this also when I profiled bug 1391710 a while ago.

Comment 5

[Tooru Fujisawa [:arai]]

Comment on attachment 8930989 [details] [diff] [review]
Patch

Review of attachment 8930989 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: js/src/vm/RegExpShared.h
@@ +273,5 @@
>      bool init();
>  
>      bool empty() const { return set_.empty(); }
>  
> +    RegExpShared* maybeGet(JSAtom* source, RegExpFlag flags) {

`lookup` seems to be const method, so this method can also be const?

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/779c4b955ba2
Avoid repeated calls to ParsePatternSyntax by using RegExpShared more. r=arai

Comment 7

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/779c4b955ba2

Comment 8

[Jan de Mooij [:jandem]]

Lol, this doubled our score on Dromaeo's object-regexp test (from ~1000 points to ~2200 points).

https://arewefastyet.com/#machine=35&view=single&suite=dromaeo&subtest=dromaeo-object-regexp

Comment 9

[Honza Bambas (:mayhemer)]

\   /
 \O/