[V8] Array.prototype.splice ~100x slower in V3 with symbol polyfill

[jimmydief]

This seems like it may be similar to #377. We recently discovered a perf issue after upgrading to v3.2.1 from v2 and tracked it down to a call to Array.prototype.splice that was occurring within a loop. Testing the performance of that method in isolation in environments with core-js/features/symbol applied revealed a similar issue.

I created a sample benchmark/repro that shows the relative performance with different versions.

[zloirock]

core-js@3 add a polyfill of Array.prototype.splice with @@species support. Yes, it's seriously slower than native implementations. Because of the performance issue, it wasn't added in 2014-2015 when it was possible, but only in this year, when all modern engines support @@species natively and it's a performance problem mainly for old engines like IE11. Or you have this problem in modern engines?

[zloirock]

Hmmm... Interesting example. Seems it's also an issue for modern V8. core-js does not replace Array.prototype.splice, it's still native, but we have performance degradation. I have no ideas.

Maybe one more kind of engine deoptimizations like #306? cc @schuay

[jimmydief]

@zloirock Thanks for the response. I see this behavior in Chrome 77 and Node 12.13.0. The issue seemed to be less significant in Firefox/Safari but the regression is still very much noticeable.

[zloirock]

I found the problem place. It's calling of this test. We try to use Array.prototype.splice on an array with changed @@species. Seems V8 sees that we can do something like that and after that uses only a deoptimized version. Yes, it's an equal of #306.

It's V8 bug, not a core-js. I don't think that I can do something here because even removing this polyfill will be a breaking change. You could write it to V8 bug tracker.

A solution for you - avoid core-js/modules/es.array.splice module (maybe also could be required to disable other methods which use @@species on Array.prototype, but unlikely). You can do it with @babel/preset-env, useBuiltIns option and a blacklist.

cc @littledan

[zloirock]

If I understood V8 source correctly, it's IsArraySpeciesProtectorCellInvalid, so it also affects Array#{ filter, map } :-(

[schuay]

Recent V8 versions also have a --trace-protector-invalidation flag that might help demonstrate what's going on:

out/debug/d8 --trace-protector-invalidation
d8> /./.constructor = () => {}
Invalidating protector cell RegExpSpeciesLookupChainProtector() => {}

I would still like to understand why core-js must touch these prototypes / species symbols. Above you said 'removing this polyfill will be a breaking change.' Could you explain (perhaps off-thread since this is slightly off-topic) what the polyfill does and why it must be applied even in recent V8 versions that should already implement the latest version of the spec?

Perhaps we can figure out a way to implement these polyfills without hitting all of V8's slow paths.

[zloirock]

@schuay it's a polyfill for feature, which causes this deoptimization (in this case - @@species support in Array#splice). Without calling this feature (Array#splice on an object where @@species is not Array), we can't determinate is this feature supported or not.

[zloirock]

Digging the V8 code, since Array#splice is generic, I found a possible workaround - calling Array#splice on a usual object with custom @@species instead of an array. (Really strange deoptimization 🤦.)

[mathiasbynens]

Digging the V8 code, since Array#splice is generic, I found a possible workaround - calling Array#splice on a usual object with custom @@species instead of an array. (Really strange deoptimization 🤦.)

Anything that avoids changing built-in prototypes (like Array.prototype.constructor) would certainly help.

[schuay]

Yes that certainly helps. We only care about constructor and @@species on relevant objects. See https://cs.chromium.org/chromium/src/v8/src/objects/lookup.cc?l=235&rcl=4f7ba9bb11d0641bd17655286f81106544790479.

E.g. for IsArraySpeciesLookupChainIntact:

• constructor modifications on an array or the array prototype
• @@species on the Array function.

[zloirock]

@mathiasbynens in this case, we do not change built-in prototypes, we change .constructor property on an instance.

core-js is a polyfill - its job is to add or to change something on built-in prototypes :-D

[zloirock]

@schuay but since RegExp methods are not generics - this approach will not work for them...

[schuay]

core-js is a polyfill - its job is to add or to change something on built-in prototypes :-D

The point is not to make these changes only when needed, i.e. not during feature detection.

[zloirock]

@schuay we don't do it on feature detection.

[schuay]

@schuay we don't do it on feature detection.

array.constructor is overwritten here:

core-js/packages/core-js/internals/array-method-has-species-support.js

Lines 4 to 15 in 34a8a4d

	var SPECIES = wellKnownSymbol('species');
	module.exports = function (METHOD_NAME) {
	return !fails(function () {
	var array = [];
	var constructor = array.constructor = {};
	constructor[SPECIES] = function () {
	return { foo: 1 };
	};
	return array[METHOD_NAME](Boolean).foo !== 1;
	});
	};

[zloirock]

@schuay I know, I wrote it above, I already wrote a workaround.

[schuay]

@schuay I know, I wrote it above, I already wrote a workaround.

Sounds great 👍

@schuay but since RegExp methods are not generics - this approach will not work for them...

I'm not sure what you mean by generics, but RegExp methods do support subclassing. E.g. RegExp.prototype.test can be called on any object that has an exec method. https://tc39.es/ecma262/#sec-regexp.prototype.test

And e.g. @@split can be called on a non-RegExp object and considers @@species: https://tc39.es/ecma262/#sec-regexp.prototype-@@split

[zloirock]

@schuay not all RegExp methods are generics, only a part of them (and, if I understood correctly, which not required for us) allows doing something like in the workaround above. I'll dig it later, maybe it's possible...

[schuay]

@schuay not all RegExp methods are generics, only a part of them (and, if I understood correctly, which not required for us) allows doing something like in the workaround above. I'll dig it later, maybe it's possible...

Feel free to ping me via email or on the tracker.

[zloirock]

Seems I closed it too early. ArraySpeciesCreate creates instances of @@species only when it's called on an array, so it's not generic behavior. I didn't remember about this since I just awoke. The feature detection does not work, I should revert it. Any ideas of how we can detect it without deoptimization, @schuay @mathiasbynens?

[zloirock]

Sure, we can just detect the engine version and do not call feature detection in modern V8:

if (V8_VERSION >= 51) return true;

but it is a very bad practice and used only in the absence of any alternatives.

[schuay]

Seems I closed it too early. ArraySpeciesCreate creates instances of @@species only when it's called on an array, so it's not generic behavior

Right, it only works if IsArray returns true, so for Arrays or Proxies on Arrays. This works, although I don't know if you can assume the existence of Proxies:

function f() { 
  const ctor = {}; 
  ctor[Symbol.species] = function () { 
    return { foo: 1 }; 
  }; 

  const a = [];
  const p = new Proxy(a, {
    get: function(obj, prop) {
      if (prop == "constructor") {
        return ctor;
      }
      return obj[prop];
    }
  });

  console.log(Array.prototype.splice.call(p, Boolean).foo);
}

f();

Spec text: https://tc39.es/ecma262/#sec-arrayspeciescreate

[zloirock]

@schuay thanks, it's an interesting solution and it could work. However, 2 problems:

• Proxy supported enough good and IIRC in all popular engines proper implementations were added before @@species on Array methods. However, we have old engines where Proxy had other APIs. Also, core-js should work everywhere - even in embed partial implementations where Proxy is missed. It will seriously complicate feature detection, but it's acceptable.
• I’m afraid of another - I think that the Proxy itself could cause any deoptimization in any engines since the behavior of basic operations without the Proxy is much simpler than with.

What do you think?

[schuay]

Not sure I understand the second point, are you asking if the Proxy-based solution could somehow cause a global switch to flip to the slow path in other engines?

If so, anything is possible I guess.. But I think it's very unlikely given that no global state is modified in the proxy-based solution.

[zloirock]

@schuay yep, something like that.

Let's think about alternative solutions for tomorrow - maybe we could find something better - and if not let's apply one of the mentioned above.

[zloirock]

I added engine version detection and not calling this feature detection in modern V8. If feature detection or usage of Proxy will be required for any other case, I'll move it to the version, proposed by @schuay.

[jimmydief]

Thanks, benchmark results confirm that this worked!

[schuay]

Thanks! Are you planning to do something similar for other builtins, especially RegExp?

Edit: Just saw #679.

[MargaretKrutikova]

I experienced the same issue with v3 in Internet Explorer 11, but not in other browsers, so we had to rollback to v2 because IE 11 was completely frozen.

Is it a known issue or has anyone else experienced the same?
Appreciate any help on this, since we are currently stuck on the legacy core-js v2 and can't upgrade because we have to support IE 11.

[Nantris]

@slowcheetah could you provide some idea of what's going on with this issue? I'm confused about whether or not this is fixed.

[zloirock]

I experienced the same issue with v3 in Internet Explorer 11, but not in other browsers, so we had to rollback to v2 because IE 11 was completely frozen. Is it a known issue or has anyone else experienced the same?
Appreciate any help on this, since we are currently stuck on the legacy core-js v2 and can't upgrade because we have to support IE 11.

It's definitely not this error. Without any additional information, I can't say anything.

[jdalton]

I filed a V8 bug on this as I was recently bit by this, in a different project, as well https://bugs.chromium.org/p/v8/issues/detail?id=13202