1549035 - CallIRGenerator::isOptimizableConstStringSplit checks for String_split instead of StringSplitString

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[André Bargull [:anba]]

Test case, run with --cache-ir-stubs=call.

var expected = 2;
for (var i = 0; i < 100; ++i) {
    if (i === 50) {
        expected = 0;
        String.prototype[Symbol.split] = function() {
            return [];
        };
    }
    var r = "ab".split("");
    assertEq(r.length, expected);
}

Fails with:

/tmp/s.js:10:5 Error: Assertion failed: got 2, expected 0
Stack:
  @/tmp/s.js:10:5

Comment 1

[Iain Ireland [:iain]]

Oof. This is a good catch. Thanks!

The motivation behind the change was to prevent const string split from being completely useless. Since it's not actually safe to do so, we should just get rid of const string split. (Conveniently, we were already considering getting rid of it.)

Comment 2

[André Bargull [:anba]]

The issue here is just that [1] is incorrect, because for const-string splitting we don't need to optimise the call to String_split, but instead the nested call to StringSplitString in String_split. So applying the following patch should fix this issue.

And wouldn't removing const string splitting completely also mean that the const-string split code in MCallOptimize needs to be removed, which then means we'll get considerably slower compared to V8 (because V8 uses a cache for string-split)? (The motivation behind optimising const-string split is that some websites save their data as a large string and are then calling staticData.split(someSeparator) to retrieve the single elements in the stored data.)

[1] https://searchfox.org/mozilla-central/rev/e7d9a8749303b39dadcc0e18ea0d60a570a68145/js/src/jit/CacheIR.cpp#5403-5405

diff --git a/js/src/jit/CacheIR.cpp b/js/src/jit/CacheIR.cpp
--- a/js/src/jit/CacheIR.cpp
+++ b/js/src/jit/CacheIR.cpp
@@ -5394,18 +5394,18 @@ bool CallIRGenerator::isOptimizableConst
     return false;
   }
 
   // And we are calling a function in the current realm...
   if (calleeFunc->realm() != cx_->realm()) {
     return false;
   }
 
-  // Which is the String split self-hosted function
-  if (!IsSelfHostedFunctionWithName(calleeFunc, cx_->names().String_split)) {
+  // Which is the String split intrinsic function.
+  if (!IsNativeFunction(calleeFunc, intrinsic_StringSplitString)) {
     return false;
   }
 
   // Then this might be a call of the form:
   //  "literal list".split("literal separator")
   // If so, we can cache the result and avoid having to perform the operation
   // each time.
   return true;

Comment 3

[Iain Ireland [:iain]]

Attachment: Bug 1549035: Remove const string split optimization r=tcampbell

Comment 4

[André Bargull [:anba]]

See bug 688219 and more recently bug 1366377 for why const-string split is optimised.

Comment 5

[Iain Ireland [:iain]]

We shouldn't get any slower, because this optimization has been broken since we first introduced String_split as part of ES6 work in bug 887016. To fill you in a little:

The problem is that intrinsic_StringSplitString is only called inside String_split. If we attach a stub for intrinsic_StringSplitString, it will therefore be attached to the self-hosted String_split function, rather than the user script. ConstStringSplit only works when the arguments are constant, which means that our existing implementation (which looked for intrinsic_StringSplitString) would break as soon as a second call to string_split was made with different arguments, anywhere in the global.

The MCallOptimize code is dependent on finding a baseline stub with the template object. So if the baseline IC doesn't do anything, the Ion optimization also won't do anything.

The change you are proposing is what the code looked like before this patch. I moved to the version with the bug because the old version wasn't doing anything. The buggy version did give a small speedup on highly targeted microbenchmarks, but nothing to write home about. It doesn't move the needle at all on Speedometer.

Here's a testcase to demonstrate the fragility of the existing optimization:

// With this loop commented out, this testcase runs in ~5.8 seconds on my machine.
// With this loop included, it takes 7+ seconds.
// Ensure that String_split has been baseline compiled and attached a stub.
for (var i = 0; i < 15; i++) {
    "".split("");
}

function foo() {
    // We need enough iterations to reach the higher tier of Ion compilation,
    // because String_split is too large to count as a "small function" 
    for (var i = 0; i < 200000; i++) {
	var array = "several words separated by spaces".split(" ");
	assertEq(array[1].length, "words".length);
    }
}

foo();

Note: it takes ~4.3 seconds for 100000 iterations, so the optimization does give a pretty solid speedup when it triggers: somewhere in the vicinity of 50%, if my quick math is correct. We should probably open a bug to redesign this from scratch more robustly.

Comment 6

[Iain Ireland [:iain]]

Opened bug 1549566 for a more robust redesign.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a40ff03e71e3
Remove const string split optimization r=tcampbell

Comment 8

[Jan de Mooij [:jandem]]

We should also consider doing a simple per-Realm cache with a few entries (purged on GC) in the C++ code, so JITs don't have to know about this.

Comment 9

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/a40ff03e71e3