The RegExp constructor can spend a lot of time under CheckPatternSyntax

RESOLVED FIXED in Firefox 59

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks 3 bugs)

unspecified
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

2 years ago
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();
Assignee

Comment 1

2 years ago
(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?
Flags: needinfo?(arai.unmht)
(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.
Flags: needinfo?(arai.unmht)
Assignee

Comment 3

2 years ago
Posted patch PatchSplinter Review
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();
Attachment #8930989 - Flags: review?(arai.unmht)
Assignee

Comment 4

2 years ago
I saw this also when I profiled bug 1391710 a while ago.
Blocks: 1391710
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?
Attachment #8930989 - Flags: review?(arai.unmht) → review+

Comment 6

2 years ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/779c4b955ba2
Avoid repeated calls to ParsePatternSyntax by using RegExpShared more. r=arai

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/779c4b955ba2
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Assignee

Comment 8

2 years ago
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
\   /
 \O/
Assignee

Updated

2 years ago
Blocks: 1426796
You need to log in before you can comment on or make changes to this bug.