1831314 - Consider rewriting some self-hosted RegExp builtins in C++

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P2)

Description

[Jan de Mooij [:jandem]]

Self-hosting probably makes sense for some of the functions that (can) take a callback argument like replace, but for exec and test we can likely get rid of some overhead by implementing in C++ and optimizing the builtin in the JITs.

The main difficulty is updating the .lastIndex value for global/sticky regular expressions, but that should be doable with CacheIR now.

Comment 1

[Jan de Mooij [:jandem]]

I started prototyping this for RegExpBuiltinExec, because that's near the bottom of the call stack. Implementing it in C++ is nice so far, it lets us get rid of some layers of abstraction.

Comment 2

[Jan de Mooij [:jandem]]

I have some WIP patches to port these to C++:

• RegExpBuiltinExec
• RegExpExec
• RegExp_prototype_Exec

Next step is to see how this affects Speedometer.

These can mostly share the same CacheIR code for the fast path. After this there are other self-hosted functions one layer up that we might be able to port to C++ too (RegExpTest, maybe RegExpMatch).

Comment 3

[Jan de Mooij [:jandem]]

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

• RegExp_prototype_Exec

This one is a bit of a problem because it's sometimes polymorphic. Octane-regexp calls it with a string-or-undefined argument, so my patches regressed our score on that test a lot, and a Speedometer 3 test (I think React-Stockcharts) calls it with an object argument (hard to support because side-effects).

RegExp_prototype_Exec is a small wrapper around RegExpBuiltinExec so let's start with just RegExpBuiltinExec and RegExpExec. This improves our Octane-regexp score by a few hundred points and should be a small win on Speedometer.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Bug 1831314 part 1 - Factor out SetLastIndex helper function. r?iain!

We want to reuse this in the next patch.

Also add an isInitialShape fast path to it, which turned out to be missing a check
for is-still-writable.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Bug 1831314 part 2 - Port RegExpBuiltinExec to C++. r?iain!

This is faster than the self-hosted implementation, especially with the JIT
optimizations added in later patches.

IC and Ion code will call the RegExpBuiltinExecMatchRaw and RegExpBuiltinExecTestRaw
functions directly.

Depends on D177504

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Bug 1831314 part 3 - Port RegExpExec to C++. r?iain!

Usually .exec will be the original RegExp.prototype.exec function, and in that case
we can call RegExpBuiltinExec.

Depends on D177505

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Bug 1831314 part 4 - Optimize RegExpBuiltinExec and RegExpExec in CacheIR. r?iain!

Depends on D177506

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Bug 1831314 part 5 - Transpile RegExpBuiltinExec* ops. r?iain!

These are very similar to RegExpMatcher and RegExpTester, except that we have some
additional code for reading/writing lastIndex.

The next patch will remove RegExpTester.

Depends on D177507

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Bug 1831314 part 6 - Remove now-unused RegExpTester. r?iain!

Depends on D177508

Comment 10

[Jan de Mooij [:jandem]]

Speedometer 3 comparison:

https://treeherder.mozilla.org/perfherder/comparesubtest?originalProject=try&newProject=try&newRevision=e3522e541d50b0bfd0b8e2bf7adb2f44a08f2a61&originalSignature=4586009&newSignature=4586009&framework=13&application=firefox&originalRevision=5da365d15093bdec368e2c08e8782a8dc2f78dcc&page=1

Sorting sub-tests by confidence shows a number of high-confidence 2-4% improvements.

Comment 11

[Markus Stange [:mstange]]

Very nice! But huh, Backbone got slower in some places! I was expecting Backbone to benefit the most.

Comment 12

[Jan de Mooij [:jandem]]

Attachment: Bug 1831314 part 7 - Split RegExpBuiltinExec and RegExpExec into separate versions for forTest. r?iain!

Callers typically pass a constant true/false so we can get rid of a CacheIR guard in this case.

Depends on D177509

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/85a3c14c83a5
part 1 - Factor out SetLastIndex helper function. r=iain
https://hg.mozilla.org/integration/autoland/rev/ebe1864b215d
part 2 - Port RegExpBuiltinExec to C++. r=iain
https://hg.mozilla.org/integration/autoland/rev/0434baa80ba4
part 3 - Port RegExpExec to C++. r=iain
https://hg.mozilla.org/integration/autoland/rev/534215665c5f
part 4 - Optimize RegExpBuiltinExec and RegExpExec in CacheIR. r=iain
https://hg.mozilla.org/integration/autoland/rev/ad69ff89c369
part 5 - Transpile RegExpBuiltinExec* ops. r=iain
https://hg.mozilla.org/integration/autoland/rev/c082236340cb
part 6 - Remove now-unused RegExpTester. r=iain
https://hg.mozilla.org/integration/autoland/rev/a07b673c7ed0
part 7 - Split RegExpBuiltinExec and RegExpExec into separate versions for forTest. r=iain

Comment 14

[Iulian Moraru]

Backed out for causing dt failure on browser_script_command_execute_basic.js.

• Push with failures

• Failure log

• Backout link

[task 2023-05-11T16:29:59.036Z] 16:29:59     INFO - TEST-PASS | devtools/shared/commands/script/tests/browser_script_command_execute_basic.js | property 'input'undefined - 
[task 2023-05-11T16:29:59.038Z] 16:29:59     INFO - checking object for property 'result'undefined
[task 2023-05-11T16:29:59.039Z] 16:29:59     INFO - TEST-PASS | devtools/shared/commands/script/tests/browser_script_command_execute_basic.js | property 'type'undefined - 
[task 2023-05-11T16:29:59.040Z] 16:29:59     INFO - Buffered messages finished
[task 2023-05-11T16:29:59.045Z] 16:29:59     INFO - TEST-UNEXPECTED-FAIL | devtools/shared/commands/script/tests/browser_script_command_execute_basic.js | no eval exception - 
[task 2023-05-11T16:29:59.046Z] 16:29:59     INFO - Stack trace:
[task 2023-05-11T16:29:59.046Z] 16:29:59     INFO - chrome://mochikit/content/browser-test.js:test_ok:1584
[task 2023-05-11T16:29:59.047Z] 16:29:59     INFO - chrome://mochitests/content/browser/devtools/shared/commands/script/tests/browser_script_command_execute_basic.js:doEagerEvalWithSideEffectMonkeyPatched:603
[task 2023-05-11T16:29:59.048Z] 16:29:59     INFO - chrome://mochitests/content/browser/devtools/shared/commands/script/tests/browser_script_command_execute_basic.js:null:89
[task 2023-05-11T16:29:59.049Z] 16:29:59     INFO - chrome://mochikit/content/browser-test.js:handleTask:1133
[task 2023-05-11T16:29:59.050Z] 16:29:59     INFO - chrome://mochikit/content/browser-test.js:_runTaskBasedTest:1205
[task 2023-05-11T16:29:59.051Z] 16:29:59     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1347
[task 2023-05-11T16:29:59.052Z] 16:29:59     INFO - chrome://mochikit/content/browser-test.js:nextTest/<:1122
[task 2023-05-11T16:29:59.052Z] 16:29:59     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/<:1056
[task 2023-05-11T16:29:59.056Z] 16:29:59     INFO - TEST-PASS | devtools/shared/commands/script/tests/browser_script_command_execute_basic.js | no helper result - 

Comment 15

[Jan de Mooij [:jandem]]

(In reply to Iulian Moraru from comment #14)

Backed out for causing dt failure on browser_script_command_execute_basic.js.

This is a devtools eager evaluation failure. The call to (overridden) exec functions in RegExpExec needs to be marked as CallReason::CallContent.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/181d38478102
part 1 - Factor out SetLastIndex helper function. r=iain
https://hg.mozilla.org/integration/autoland/rev/1c9a6dc02923
part 2 - Port RegExpBuiltinExec to C++. r=iain
https://hg.mozilla.org/integration/autoland/rev/bf65109b6c41
part 3 - Port RegExpExec to C++. r=iain
https://hg.mozilla.org/integration/autoland/rev/68dd64a6dbe8
part 4 - Optimize RegExpBuiltinExec and RegExpExec in CacheIR. r=iain
https://hg.mozilla.org/integration/autoland/rev/9e2a3f04e0ac
part 5 - Transpile RegExpBuiltinExec* ops. r=iain
https://hg.mozilla.org/integration/autoland/rev/05b3296dcc61
part 6 - Remove now-unused RegExpTester. r=iain
https://hg.mozilla.org/integration/autoland/rev/5ac14c27f807
part 7 - Split RegExpBuiltinExec and RegExpExec into separate versions for forTest. r=iain

Comment 17

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/181d38478102
https://hg.mozilla.org/mozilla-central/rev/1c9a6dc02923
https://hg.mozilla.org/mozilla-central/rev/bf65109b6c41
https://hg.mozilla.org/mozilla-central/rev/68dd64a6dbe8
https://hg.mozilla.org/mozilla-central/rev/9e2a3f04e0ac
https://hg.mozilla.org/mozilla-central/rev/05b3296dcc61
https://hg.mozilla.org/mozilla-central/rev/5ac14c27f807