RegExp polyfill path may be triggering on new V8 versions

[littledan]

Ideally, core.js should either feature-detect that V8 implements ES2015 RegExp semantics and therefore leave RegExp alone, or V8 should fix itself so that it does implement those semantics. However, @schuay reports at babel/babel#5771 that V8 triggers core.js to modify RegExp.

[zloirock]

As I already wrote, I was not able to reproduce this issue. Here is a feature detection:

var re1 = /a/g;
var re2 = /a/g;
re2[Symbol.match] = false;
RegExp(re1) == re1 && RegExp(re2) != re2 && RegExp(re1, 'i') == '/a/i';

RegExp constructor (and RegExp.prototype.constructor) polyfilled only in case if it's failed, but V8 passes it for a long time.

But now we have one more issue. In the article I see the note:

To take the fast path:

• regexp must be unmodified (no added, deleted, or modified properties)
• regexp.prototype must be unmodified
• regexp.lastIndex must be a simple non-negative integer — not an object, not a getter.

Disallowing modification of RegExp.prototype is disallowing polyfilling of RegExp-related features. The first example - updated algorithm of String#matchAll which should use RegExp#@@matchAll method. So what we should do in this case?

[zloirock]

Seems related https://bugs.chromium.org/p/chromium/issues/detail?id=977382

[schuay]

https://crbug.com/977382 is also related to regexp fast paths, but probably not relevant to this issue.

Things have gotten a bit better than when this issue was originally opened. It is now possible to modify existing but irrelevant functions on the prototype without falling off the fast path (e.g.: String.prototype.match only checks @@match and exec).

Unfortunately, adding a new property to the prototype will still trigger the slow path.

On your side, there's not much you can do except be conservative about modifying the regexp prototype.

On the V8 side, the goal is still to switch to a smarter fast path check that only checks relevant properties, ignoring all other changes on the prototype. The relevant V8 bug is https://crbug.com/v8/5577.

[danielhusar]

Hey guys, I think I have repro case for this issue: (thanks to @rickharrison)

repository: https://github.com/danielhusar/core-js-performance
demo: https://core-js-preformance.netlify.com/

Follow the steps in the demo link.

[schuay]

Hey guys, I think I have repro case for this issue

Indeed. The slowdown even persists beyond page reload within the same tab.

[danielhusar]

The scary part is that you can include this polyfill even in iframe (I only tried sourceless), and your whole tab (parent window, etc..) will get slow paths, probably forever.

[schuay]

I hope to look into a fix for this soon (definitely the cross-context leak of state, perhaps also a more permissive fast path check).

[schuay]

https://crrev.com/c/1695465 should fix the cross-navigation and page-reload pollution for the example above (https://core-js-preformance.netlify.com/).

[schuay]

And https://crrev.com/c/1706056 allows benign changes to the RegExp prototype on the fast path. Currently limited to a subset of regexp builtins, but can be extended to all builtins with a bit of work.

[ecc521]

I'm also noticing extreme RegExp performance problems - after including core-js, performance of regexp plunged 120x - with rather devastating implications for my search tools.

Doing some testing, I measured about 1/3rd of a millisecond to split a 20 character string using the really simple regexp /[, ]+/ - Flat out sad. (I created a jsperf showing the impact here. It only shows a 20x difference for me, so either my use case is probably hit more than normal, or core-js is doing something else that gives V8 even more trouble)

There doesn't appear to be much progress on the V8 side, which is a little surprising given just how slow the slow path is. I'll see if I can do anything there.

I'd also like to ask: Is there any reason that core-js needs to modify the RegExp prototype in more recent versions of V8? I'm probably missing something, but V8 tends to keep up with the standards game relatively well.

[schuay]

I'd also like to ask: Is there any reason that core-js needs to modify the RegExp prototype in more recent versions of V8? I'm probably missing something, but V8 tends to keep up with the standards game relatively well.

+1 that is my question as well. If there is any way to avoid it, one should not touch builtin prototypes.

There doesn't appear to be much progress on the V8 side, which is a little surprising given just how slow the slow path is. I'll see if I can do anything there.

There is no work planned to speed up the slow path. The assumption we are operating on is that if you fall off the fast path, you're out of luck. Results are still correct, but performance will likely be unimpressive.

We do have ideas on how to rewrite RegExp builtins to widen the fast path further, but that would be a larger project. At the moment, I don't see it happening any time soon. Pragmatically, I think the best way to proceed is to work with core-js and figure out if modifying the prototype is necessary at all.

[zloirock]

#677 one more problem like that.

[ecc521]

100x is similar to the slowdown that I was seeing on Array.prototype.slice.
Usage counters have been added to the V8 bug report, so some progress may happen in the future.

[zloirock]

The main performance problem here caused by the same code as in #677 - @@species support detection.

[zloirock]

Could you check this patch on your cases? On my simple test case, it works fine.

[ecc521]

I'm seeing a 114x performance improvement. It looks like this fixes the deoptimization issue.

[zloirock]

@schuay what about your test case?

[zloirock]

Adding any new RegExp-related polyfill will cause this issue again since it's a modification of RegExp.prototype, so it should be fixed on V8 side. Waiting for this fix or a new RegExp-related proposal.

[schuay]

@schuay what about your test case?

I lost context here, which test case do you mean?

Adding any new RegExp-related polyfill will cause this issue again since it's a modification of RegExp.prototype, so it should be fixed on V8 side. Waiting for this fix or a new RegExp-related proposal.

If possible, we should fix the current issue first and worry about future changes when they come.

[zloirock]

I lost context here, which test case do you mean?

Which initially was a reason for this issue?

[schuay]

I lost context here, which test case do you mean?
Which initially was a reason for this issue?

Well, multiple loosely-related things have been discussed on this issue so far ;)

The original bug report was apparently for the es6.regexp.constructor polyfill. I don't think the fix in a9aef3c will affect the logic in https://github.com/zloirock/core-js/blob/8714aecf822eb1b361c7a76a69a77e8862fe5993/packages/core-js/modules/es.regexp.constructor.js.

I'm not sure the original issue is still valid as you've mentioned previously. The associated feature detection seems to work fine on a current V8:

core-js/packages/core-js/modules/es.regexp.constructor.js

Line 26 in 8714aec

return NativeRegExp(re1) != re1 || NativeRegExp(re2) == re2 || NativeRegExp(re1, 'i') != '/a/i';

V8 version 8.0.0 (candidate)
d8> const re1 = /a/g
d8> const re2 = /a/g
d8> re2[Symbol.match] = false;
d8> RegExp(re1) != re1
false
d8> RegExp(re2) == re2
false
d8> RegExp(re1, 'i') != '/a/i'
false