1655465 - Optimise String.prototype.replace and String.prototype.split in CacheIR and Warp

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, task, P1)

Description

[André Bargull [:anba]]

Support StringReplaceString, StringSplitString, and ObjectHasPrototype in CacheIR and Warp. Plus transpile any remaining CacheIR ops used within String.prototype.replace and String.prototype.split (i.e. LoadValueTag and GuardTagNotEqual).

To ensure the generated code looks sensible (*), also apply additional optimisations to MCompare and replace the GetBuiltinPrototype() and GetBuiltinConstructor() intrinsics with a new JSOp.

---

(*) The generated code will look almost sensible, except that it's still cluttered with MBail instructions in unreachable code. This is probably caused by https://searchfox.org/mozilla-central/rev/cf561cece0ca9aeaf0202e68699836d957d0c670/js/src/jit/WarpBuilder.cpp#3094-3095.

Comment 1

[André Bargull [:anba]]

Attachment: Bug 1655465 - Part 1: Transpile LoadValueTag in Warp. r=jandem!

LoadValueTag is used when comparing strictly different types. This happens in
the string built-ins when the this-value is a primitive string and it is
compared against null or undefined.

Value tags are represented as MIRType::Int32 and folding is supported to be
able to optimise away the checks after inlining.

Comment 2

[André Bargull [:anba]]

Attachment: Bug 1655465 - Part 2: Test node is commutative before swapping the operands. r=jandem!

Swapping bi's operands should depend on bi itself being commutative.

Drive-by change: Use std::swap to swap the operands.

Depends on D84976

Comment 3

[André Bargull [:anba]]

Attachment: Bug 1655465 - Part 3: Use BytecodeUtil.h helpers to test for equality operations. r=jandem!

Depends on D84977

Comment 4

[André Bargull [:anba]]

Attachment: Bug 1655465 - Part 4: Transpile GuardTagNotEqual in Warp. r=jandem!

This is part two to support comparing strictly different types in Warp.

The operation doesn't have any result, so when folding it, MNop is used to
optimise it away when the value tags are already different at compile time.

Depends on D84978

Comment 5

[André Bargull [:anba]]

Attachment: Bug 1655465 - Part 5: Rename `tryFoldTypeOf` to `tryFoldConstantTypeOf`. r=jandem!

The next part will add a different tryFoldTypeOf() method, so rename the
existing one to avoid any confusion.

Drive-by change: Correct the access specifiers.

Depends on D84980

Comment 6

[André Bargull [:anba]]

Attachment: Bug 1655465 - Part 6: Fold MCompare with typeof to check the value tag. r=jandem!

String built-ins use typeof thing === "string" to check for string values.
These were compiled to LTypeOfV followed by a string comparison. Using
MLoadValueTag from part 1 can make this more efficient.

Depends on D84982

Comment 7

[André Bargull [:anba]]

Attachment: Bug 1655465 - Part 5: Support ObjectHasPrototype in CacheIR. r=jandem!

Inling ObjectHasPrototype() using the GuardProto CacheIR op for the normal
case when the prototype chain wasn't modified.

Also change intrinsic_ObjectHasPrototype() to expect that both objects are
NativeObject, because then we don't need to handle proxies in CacheIR.

Drive-by change:

• Use staticPrototype() in ObjectOperations-inl.h instead of effectively inlining it.

Depends on D84983

Comment 8

[André Bargull [:anba]]

Attachment: Bug 1655465 - Part 6: Support StringReplaceString in CacheIR and Warp. r=jandem!

Drive-by change:

• Remove a bogus resumeAfter(), MStringReplace is never effectful.

Depends on D84984

Comment 9

[André Bargull [:anba]]

Attachment: Bug 1655465 - Part 7: Avoid MToString on string-typed inputs in Warp. r=jandem!

Warp can't currently fold away MToString, as a workaround handle the already
string case in WarpBuilder.

Depends on D84987

Comment 10

[André Bargull [:anba]]

Attachment: Bug 1655465 - Part 8: Support StringSplitString in CacheIR and Warp. r=jandem!

Drive-by change:

• Make intrinsic_StringSplitString a static function.

Depends on D84988

Comment 11

[André Bargull [:anba]]

Attachment: Bug 1655465 - Part 9: Change JSOp::FunctionProto to JSOp::BuiltinObject. r=jandem!

Callers to GetBuiltinPrototype() rely on inlining the function itself plus
optimising the object access, so that the property value is directly seen as a
constant in the compiler. By changing GetBuiltinPrototype() and
GetBuiltinConstructor() to be directly translated into a JSOp, we can avoid
heavily relying on the compiler to optimise these two functions.

The new opcode replaces the existing JSOp::FunctionProto opcode. It doesn't
use JSProtoKey directly in order to help jsparagus (bug 1622530), but instead
uses its own set of mapping from enum values to built-in prototypes and
constructors.

This change also resolves bug 1377264.

Depends on D84989

Comment 12

[Jan de Mooij [:jandem]]

That GetBuiltinPrototype change is great! I ran into that a few weeks ago when optimizing the RegExp intrinsics, and the old code was surprising.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8fe934b1ec0a
Part 1: Transpile LoadValueTag in Warp. r=jandem
https://hg.mozilla.org/integration/autoland/rev/b395e15e6453
Part 2: Test node is commutative before swapping the operands. r=jandem
https://hg.mozilla.org/integration/autoland/rev/bad8617d0562
Part 3: Use BytecodeUtil.h helpers to test for equality operations. r=jandem
https://hg.mozilla.org/integration/autoland/rev/1b65fe996077
Part 4: Transpile GuardTagNotEqual in Warp. r=jandem
https://hg.mozilla.org/integration/autoland/rev/7e65234359d8
Part 5: Rename `tryFoldTypeOf` to `tryFoldConstantTypeOf`. r=jandem
https://hg.mozilla.org/integration/autoland/rev/d8a3835a65e7
Part 6: Fold MCompare with typeof to check the value tag. r=jandem
https://hg.mozilla.org/integration/autoland/rev/453eb1a0a816
Part 7: Support ObjectHasPrototype in CacheIR. r=jandem
https://hg.mozilla.org/integration/autoland/rev/cecabf647d61
Part 8: Support StringReplaceString in CacheIR and Warp. r=jandem
https://hg.mozilla.org/integration/autoland/rev/6f5eafbf3525
Part 9: Avoid MToString on string-typed inputs in Warp. r=jandem
https://hg.mozilla.org/integration/autoland/rev/b57c53d012b4
Part 10: Support StringSplitString in CacheIR and Warp. r=jandem
https://hg.mozilla.org/integration/autoland/rev/c47720118613
Part 11: Change JSOp::FunctionProto to JSOp::BuiltinObject. r=jandem

Comment 14

[Dorel Luca [:dluca]]

Backed out 11 changesets (bug 1655465) for build bustage in builds/worker/checkouts/gecko/js/src/jit/MIR.cpp. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=311630553&repo=autoland&lineNumber=31233

Log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=311630575&repo=autoland&lineNumber=3716

Log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=311630574&repo=autoland&lineNumber=22479

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&revision=c477201186138888f467cb1c996bd467eb57f7d7

Backout:
https://hg.mozilla.org/integration/autoland/rev/8c8ff40a0a104cf8a5165a57dcd10aa7c9c48156

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4dbe80f41370
Part 1: Transpile LoadValueTag in Warp. r=jandem
https://hg.mozilla.org/integration/autoland/rev/737e3a6415de
Part 2: Test node is commutative before swapping the operands. r=jandem
https://hg.mozilla.org/integration/autoland/rev/66ff72ea169b
Part 3: Use BytecodeUtil.h helpers to test for equality operations. r=jandem
https://hg.mozilla.org/integration/autoland/rev/bc2378fcaf0f
Part 4: Transpile GuardTagNotEqual in Warp. r=jandem
https://hg.mozilla.org/integration/autoland/rev/367207c1cece
Part 5: Rename `tryFoldTypeOf` to `tryFoldConstantTypeOf`. r=jandem
https://hg.mozilla.org/integration/autoland/rev/7527a8726d9c
Part 6: Fold MCompare with typeof to check the value tag. r=jandem
https://hg.mozilla.org/integration/autoland/rev/1682c7ee28f9
Part 7: Support ObjectHasPrototype in CacheIR. r=jandem
https://hg.mozilla.org/integration/autoland/rev/8b030b7d13ab
Part 8: Support StringReplaceString in CacheIR and Warp. r=jandem
https://hg.mozilla.org/integration/autoland/rev/5ead4048955f
Part 9: Avoid MToString on string-typed inputs in Warp. r=jandem
https://hg.mozilla.org/integration/autoland/rev/a5491d62d560
Part 10: Support StringSplitString in CacheIR and Warp. r=jandem
https://hg.mozilla.org/integration/autoland/rev/6567d54b63c8
Part 11: Change JSOp::FunctionProto to JSOp::BuiltinObject. r=jandem

Comment 16

[Andreea Pavel [:apavel]]

Backed out for breaking SM builds

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception&searchStr=spidermonkey&revision=6567d54b63c8271853f9f33548f7d61f596a8314&selectedTaskRun=AHAO8DMQRH2BTSYuFlbzWg.0

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=311684869&repo=autoland&lineNumber=4939

Backout: https://hg.mozilla.org/integration/autoland/rev/801b59414882378eff04a7e5ac0b27a8c91fd59d

Comment 17

[Andreea Pavel [:apavel]]

Also caused jit test failures: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=311688205&repo=autoland&lineNumber=1739

Comment 18

[Tom S. (please needinfo tschuster)]

Support for GuardTagNotEqual would be great to have. This is also used by (obj = GuardToArrayIterator(this)) === null in ArrayIteratorNext.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b1062d8261fc
Part 1: Transpile LoadValueTag in Warp. r=jandem
https://hg.mozilla.org/integration/autoland/rev/1dc74f4f2413
Part 2: Test node is commutative before swapping the operands. r=jandem
https://hg.mozilla.org/integration/autoland/rev/c0d82360f73b
Part 3: Use BytecodeUtil.h helpers to test for equality operations. r=jandem
https://hg.mozilla.org/integration/autoland/rev/35399bebf4b6
Part 4: Transpile GuardTagNotEqual in Warp. r=jandem
https://hg.mozilla.org/integration/autoland/rev/5b74237b6a7e
Part 5: Support ObjectHasPrototype in CacheIR. r=jandem
https://hg.mozilla.org/integration/autoland/rev/2867f1a4afcb
Part 6: Support StringReplaceString in CacheIR and Warp. r=jandem
https://hg.mozilla.org/integration/autoland/rev/b958b136e2a8
Part 7: Avoid MToString on string-typed inputs in Warp. r=jandem
https://hg.mozilla.org/integration/autoland/rev/02cbce29b638
Part 8: Support StringSplitString in CacheIR and Warp. r=jandem
https://hg.mozilla.org/integration/autoland/rev/834447abbbf1
Part 9: Change JSOp::FunctionProto to JSOp::BuiltinObject. r=jandem

Comment 20

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/b1062d8261fc
https://hg.mozilla.org/mozilla-central/rev/1dc74f4f2413
https://hg.mozilla.org/mozilla-central/rev/c0d82360f73b
https://hg.mozilla.org/mozilla-central/rev/35399bebf4b6
https://hg.mozilla.org/mozilla-central/rev/5b74237b6a7e
https://hg.mozilla.org/mozilla-central/rev/2867f1a4afcb
https://hg.mozilla.org/mozilla-central/rev/b958b136e2a8
https://hg.mozilla.org/mozilla-central/rev/02cbce29b638
https://hg.mozilla.org/mozilla-central/rev/834447abbbf1