1263340 - Change flags access in RegExpBuiltinExec from getter to internal slot.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tooru Fujisawa [:arai]]

The ES draft spec changed the RegExpBuiltinExec to use internal slot for global and sticky flags.
https://github.com/tc39/ecma262/issues/489

We need to apply that change, and hopefully apply some more optimization.

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Use internal slot for global and sticky flags in RegExpBuiltinExec.

As a first step, changed global/sticky access to UnsafeGetInt32FromReservedSlot.

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: Part 1.1: (WIP) Add RegExpGetFlags to fold flags of a singleton RegExp to a constant.

Then, added RegExpGetFlags intrinsic, that can be folded into boolean constant if argument RegExp is a single object, so that either global/non-global branch in RegExpBuiltinExec can be removed while optimization.

In IonBuilder::inlineRegExpGetFlags (or somewhere else), I want to check if the argument refer to single MRegExp, but at the point of IonBuilder::inlineRegExpGetFlags it can be MPhi.  Is there any good place to check and fold it?

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Part -: Do not check global and sticky getters in RegExpPrototypeOptimizable.

Also, now we don't use global/sticky accessor, so removed that check from RegExpPrototypeOptimizable.

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: (testing) inline RegExpExec and RegExpBuiltinExec into RegExpTest

Here's the performance comparison on stringFilter (bug 1260435)

  before:     108100   ops/sec
  parts 1-2:  124274.5 ops/sec
  this patch: 133602   ops/sec

So, if the performance is very critical, we could apply this or similar optimization (#define+#include in bug 1264264 maybe?), to reduce function call + extra `forTest` branch.

Comment 5

[Hannes Verschore [:h4writer]]

Comment on attachment 8742336 [details] [diff] [review]
Part 1.1: (WIP) Add RegExpGetFlags to fold flags of a singleton RegExp to a constant.

Review of attachment 8742336 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/MCallOptimize.cpp
@@ +2117,5 @@
> +    } else {
> +        MLoadFixedSlot* load = MLoadFixedSlot::New(alloc(), rxArg, RegExpObject::flagsSlot());
> +        current->add(load);
> +        current->push(load);
> +        load->setResultType(MIRType_Int32);

I think it might be better to always push the MLoadFixedSlot.
And in MLoadFixedSlot::foldsTo add this optimization?

(Is it possible to adjust the flags of a regexp? If it is, you might want to check "dependency()" to make sure that hasn't happened)

Comment 6

[Tooru Fujisawa [:arai]]

Thank you for your feedback!
Will look into MLoadFixedSlot::foldsTo.

flags cannot be changed, so, it can be folded into boolean constant whenever the argument is a single RegExp

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Part 1.5: Fold MLoadFixedSlot on flagsSlot of MRegExp to a constant.

Added code to MLoadFixedSlot::foldsTo, there object() is already a MRegExp instruction.
The performance result is almost same as before.

Comment 8

[Hannes Verschore [:h4writer]]

Comment on attachment 8742337 [details] [diff] [review]
Part -: Do not check global and sticky getters in RegExpPrototypeOptimizable.

Review of attachment 8742337 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!

Comment 9

[Hannes Verschore [:h4writer]]

Comment on attachment 8742335 [details] [diff] [review]
Part 1: Use internal slot for global and sticky flags in RegExpBuiltinExec.

Review of attachment 8742335 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/RegExp.js
@@ +798,5 @@
>      // Step 5.d.
>      return forTest ? result !== null : result;
>  }
>  
> +// ES rev 6a13789aa9e7c6de4e96b7d3e24d9e6eba6584ad 21.2.5.2.2.

Shouldn't we keep "ES6" ?
Or use ecma262 ?

::: js/src/builtin/SelfHostingDefines.h
@@ +83,5 @@
>  #define JSITER_SYMBOLSONLY 0x40 /* exclude string property keys */
>  
>  #define TELEMETRY_DEFINE_GETTER_SETTER_THIS_NULL_UNDEFINED 25
>  
> +#define REGEXP_FLAGS_SLOT 2

We define this on two places now: here and RegExpObject::FLAGS_SLOT.
Can we merge them both? Or can we at least assert both are equal?

Comment 10

[Tooru Fujisawa [:arai]]

Thank you for reviewing :D

(In reply to Hannes Verschore [:h4writer] from comment #9)
> > +// ES rev 6a13789aa9e7c6de4e96b7d3e24d9e6eba6584ad 21.2.5.2.2.
> 
> Shouldn't we keep "ES6" ?
> Or use ecma262 ?

This change is not ES6.
How about "ES 2017 draft rev ..." ?

> ::: js/src/builtin/SelfHostingDefines.h
> @@ +83,5 @@
> >  #define JSITER_SYMBOLSONLY 0x40 /* exclude string property keys */
> >  
> >  #define TELEMETRY_DEFINE_GETTER_SETTER_THIS_NULL_UNDEFINED 25
> >  
> > +#define REGEXP_FLAGS_SLOT 2
> 
> We define this on two places now: here and RegExpObject::FLAGS_SLOT.
> Can we merge them both? Or can we at least assert both are equal?

Okay, will add assertion to RegExpObject.h.

Comment 11

[Hannes Verschore [:h4writer]]

(In reply to Tooru Fujisawa [:arai] from comment #10)
> Thank you for reviewing :D
> 
> (In reply to Hannes Verschore [:h4writer] from comment #9)
> > > +// ES rev 6a13789aa9e7c6de4e96b7d3e24d9e6eba6584ad 21.2.5.2.2.
> > 
> > Shouldn't we keep "ES6" ?
> > Or use ecma262 ?
> 
> This change is not ES6.
> How about "ES 2017 draft rev ..." ?

Yeah that is even better.

> 
> > ::: js/src/builtin/SelfHostingDefines.h
> > @@ +83,5 @@
> > >  #define JSITER_SYMBOLSONLY 0x40 /* exclude string property keys */
> > >  
> > >  #define TELEMETRY_DEFINE_GETTER_SETTER_THIS_NULL_UNDEFINED 25
> > >  
> > > +#define REGEXP_FLAGS_SLOT 2
> > 
> > We define this on two places now: here and RegExpObject::FLAGS_SLOT.
> > Can we merge them both? Or can we at least assert both are equal?
> 
> Okay, will add assertion to RegExpObject.h.

Thanks

Comment 12

[Tooru Fujisawa [:arai]]

Waldo pointed out that RegExp's flags slot can be modified by RegExp#compile, so Part 1.5 is wrong...

Comment 13

[Tooru Fujisawa [:arai]]

Also, about Part 2, it might be better keeping global/sticky accessor check, or, even add unicode there,
so that we can skip property access at other places on optimizable cases.
I suppose it's rare case that those accessors are modified, and the cost to check them won't be high, as we can check them by shape for 2nd call.

At least, @@replace could benefit from checking global/unicode there.
Will check other applicable cases.

Comment 14

[Tooru Fujisawa [:arai]]

One more thought about possible optimization.
Since sticky flag is no more customizable by accessor, we could remove `sticky` parameter from several functions,
and we can reduce the RegExpObject's compilationArray length from 8 to 4 again.

But we use different `sticky` value in RegExp#@@split optimized path, to ignore sticky flag.  So that we need another way to override sticky flag in that path if we remove `sticky` parameter.

Comment 15

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Use internal slot for sticky flag in @@replace and @@search optimized path.

Anyway, sticky flag access in other optimized path should be changed to follow the spec.
Will apply same change for bug 1264264's patch (@@replace optimized paths)

Comment 16

[Hannes Verschore [:h4writer]]

Comment on attachment 8743672 [details] [diff] [review]
Part 2: Use internal slot for sticky flag in @@replace and @@search optimized path.

Review of attachment 8743672 [details] [diff] [review]:
-----------------------------------------------------------------

Could it be that you missed some R.global and rx.global in RegExp.js ?

Comment 17

[Tooru Fujisawa [:arai]]

Thank you for reviewing :)

(In reply to Hannes Verschore [:h4writer] from comment #16)
> Comment on attachment 8743672 [details] [diff] [review]
> Part ?: Use internal slot for sticky flag in @@replace and @@search
> optimized path.
> 
> Review of attachment 8743672 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Could it be that you missed some R.global and rx.global in RegExp.js ?

There are still 3 places that accesses .global property.
  https://tc39.github.io/ecma262/#sec-get-regexp.prototype.flags
  https://tc39.github.io/ecma262/#sec-regexp.prototype-@@match
  https://tc39.github.io/ecma262/#sec-regexp.prototype-@@replace

global/sticky access is changes to internal slot only in RegExpBuiltinExec
  https://tc39.github.io/ecma262/#sec-regexpbuiltinexec

sticky property accesses replaced in this patch is the step 7 there.
Step 6 were already skipped because RegExpPrototypeOptimizable.

Comment 18

[Tooru Fujisawa [:arai]]

I'll land following first:
  Part 1: Use internal slot for global and sticky flags in RegExpBuiltinExec.
  Part ?: Use internal slot for sticky flag in @@replace and @@search optimized path.

then address remaining things (sticky parameter, more aggressive optimization, etc).  after that, will re-think about flag accessors in RegExpPrototypeOptimizable, and folding MLoadFixedSlot.

Comment 19

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3d31abc6b37d5c69fca3631377c36c09a3fa4d5b
Bug 1263340 - Part 1: Use internal slot for global and sticky flags in RegExpBuiltinExec. r=h4writer

https://hg.mozilla.org/integration/mozilla-inbound/rev/53b4512a42b434524730ddf84b6f448e0a0d3c80
Bug 1263340 - Part 2: Use internal slot for sticky flag in @@replace and @@search optimized path. r=h4writer

Comment 20

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Use internal slot for sticky flag in RegExp native functions.

based on bug 1264264's patch.

removed sticky parameter from several RegExp functions.
basically just removed custom sticky parameter and uses internal value.

the special case in this patch is RegExpSplit.
There we were using non-default sticky flag in RegExpMatcher, but we cannot do it now.
So, added regexp_construct_no_sticky intrinsic, that is almost same as RegExp(regexp, flags), but it ignores sticky flag.
It could be done in self-hosted JS by mutating flags string, but I think this way is simpler, as removing single character from string isn't low-cost.

it improves Octane RegExp score from 4206 to 4265.

also, the size of RegExpShared reduces by 4*sizeof(pointer), in compilationArray, as we don't need variant for sticky=true/false.
(comment #14 was wrong that it's not RegExpObject's size)

Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=71ddb53fb118

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3d31abc6b37d
https://hg.mozilla.org/mozilla-central/rev/53b4512a42b4

Comment 22

[Hannes Verschore [:h4writer]]

Comment on attachment 8744252 [details] [diff] [review]
Part 3: Use internal slot for sticky flag in RegExp native functions.

Review of attachment 8744252 [details] [diff] [review]:
-----------------------------------------------------------------

Wow this was a longer read than I expected, but looks good!

::: js/src/builtin/RegExp.cpp
@@ +1011,1 @@
>                    RegExpStaticsUpdate staticsUpdate, int32_t* result)

super nit: can this be on the above line now? Without going over the 100 column length?

@@ +1070,1 @@
>                        MatchPairs* maybeMatches, int32_t* result)

super nit: can this be on the above line now? Without going over the 100 column length?

@@ +1124,1 @@
>                                             nullptr, &endIndex, UpdateRegExpStatics);

super nit: can this be on the above line now? Without going over the 100 column length?

::: js/src/builtin/RegExp.h
@@ +22,5 @@
>  // Whether RegExp statics should be updated with the input and results of a
>  // regular expression execution.
>  enum RegExpStaticsUpdate { UpdateRegExpStatics, DontUpdateRegExpStatics };
>  
> +

nit: no need for this extra newline?

::: js/src/builtin/RegExp.js
@@ +591,5 @@
> +        splitter = regexp_construct_no_sticky(rx, flags);
> +    } else {
> +        // Steps 8-9.
> +        var newFlags;
> +        if (callFunction(std_String_includes, flags, "y"))

What about flags.contains("y") ?

Comment 23

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/86d33031bbd3f7173d8efbc1d5da8bb22352c53f
Bug 1263340 - Part 3: Use internal slot for sticky flag in RegExp native functions. r=h4writer

Comment 24

[Tooru Fujisawa [:arai]]

About flags, it might be nice to check all flag accessors, including RegExp.prototype.flags, so that we can use slot value directly in @@split, instead of string representation returned by RegExp.prototype.flags.

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/86d33031bbd3

Comment 26

[Tooru Fujisawa [:arai]]

Attachment: Part 4: Followup for @@split - Apply optimized path for empty string too.

changed to use optimized path for |"".split(/a/)| case.


now testing more optimization, by using flags slot directly in other places, checking all accessors in RegExpPrototypeOptimizable, but no improvement confirmed on macro benchmark :/
will try investigating some more.

Comment 27

[Till Schneidereit [:till]]

Comment on attachment 8751430 [details] [diff] [review]
Part 4: Followup for @@split - Apply optimized path for empty string too.

Review of attachment 8751430 [details] [diff] [review]:
-----------------------------------------------------------------

Huh, I somehow missed this patch, sorry for the delay.

r=me with or without comment addressed.

::: js/src/builtin/RegExp.js
@@ +636,5 @@
>  
>      // Step 16;
>      var p = 0;
>  
>      // Step 16;

Pre-existing, but: can you fix the step comments (both to get rid of the duplication of Step 16 and s/;/./) while you're here?

Comment 28

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d6db1d37aca78228a2310b19e52d1ca488091aed
Bug 1263340 - Part 4: Followup for @@split - Apply optimized path for empty string too. r=till

Comment 29

[Ryan VanderMeulen [:RyanVM]]

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

Comment 30

[Tooru Fujisawa [:arai]]

Attachment: Performance comparison with optimization for match/search/split

Fortunately, now WIP patch improves each case a bit (up to 3%) for match/search/split.
will check some more and post them.

replace needs another fix in bug 1290506 before optimization.

Comment 31

[Tooru Fujisawa [:arai]]

Attachment: Part 5: Check RegExp.prototype.flags getter in RegExpPrototypeOptimizable.

in addition to bug 1290414, added flags, so that we can skip flags getter call in RegExp#split.

Comment 32

[Tooru Fujisawa [:arai]]

Attachment: Part 6: Use flags slot value instead of RegExp.prototype.flags in RegExpSplit.

if flags accessor is not modified, we don't need to call it in optimized path,
but just access flags slot and turn sticky bit off.

regexp_construct_no_sticky is changed to regexp_construct_raw_flags, as we can calculate flags in JS side.

Comment 33

[Tooru Fujisawa [:arai]]

Attachment: Part 7: Use internal slot for flags in RegExpReplace optimizable cases.

in @@replace optimized path, changed to use flags slot,
and moved related code to RegExpReplace function.

also moved rx.global to slow path.

Comment 34

[Tooru Fujisawa [:arai]]

Attachment: Part 8: Separate RegExpSearch slow path.

@@search is already optimized, but splitting slow path into another function can improve performance for optimized case.

Comment 35

[Till Schneidereit [:till]]

Comment on attachment 8804173 [details] [diff] [review]
Part 5: Check RegExp.prototype.flags getter in RegExpPrototypeOptimizable.

Review of attachment 8804173 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 36

[Tooru Fujisawa [:arai]]

Attachment: Part 9: Add optimizable path for RegExpMatch with global flag.

applied similar optimization as @@search to @@match.
in optimized path, it calls RegExpMatcher directly and skips ToLength etc,
also separated slow path to another function.

Comment 37

[Till Schneidereit [:till]]

Comment on attachment 8804174 [details] [diff] [review]
Part 6: Use flags slot value instead of RegExp.prototype.flags in RegExpSplit.

Review of attachment 8804174 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nits addressed.

::: js/src/builtin/RegExp.cpp
@@ +544,1 @@
>          return false;

Given how the flags value is derived in JS, it's safe to just use `int32_t flags = args[1].toInt32()` here. That means you can either omit the `isNumber` assert above, or change it to `isInt32`.

Alternatively if you think this should be more robust, you can also use toNumber and do an int32 cast on the result: the only reason for this to ever be anything but a real int32 is because somehow the internal representation of the value was different, not because it really isn't an integer in the int32 range.

::: js/src/builtin/RegExp.h
@@ +89,5 @@
>  
>  /*
> + * Behaves like RegExp(pattern, flags).
> + * pattern should be a RegExp object, and flags should be a raw integer value
> + * of RegExpFlag, and should be called without |new|.

Nit: I would slightly reword this like so:

 * |pattern| should be a RegExp object, |flags| should be a raw integer value.
 * Must be called without |new|.

Comment 38

[Till Schneidereit [:till]]

Comment on attachment 8804175 [details] [diff] [review]
Part 7: Use internal slot for flags in RegExpReplace optimizable cases.

Review of attachment 8804175 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 39

[Till Schneidereit [:till]]

Comment on attachment 8804176 [details] [diff] [review]
Part 8: Separate RegExpSearch slow path.

Review of attachment 8804176 [details] [diff] [review]:
-----------------------------------------------------------------

nice! r=me

Comment 40

[Till Schneidereit [:till]]

Comment on attachment 8804177 [details] [diff] [review]
Part 9: Add optimizable path for RegExpMatch with global flag.

Review of attachment 8804177 [details] [diff] [review]:
-----------------------------------------------------------------

This looks pretty great. r=me

Comment 41

[Tooru Fujisawa [:arai]]

thank you for reviewing :D

clearing tracking flag, as this is performance thing, not wrong behavior etc.

Comment 42

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d56da58f4893b2c3ae28339e000b42c4aa896f3
Bug 1263340 - Part 5: Check RegExp.prototype.flags getter in RegExpPrototypeOptimizable. r=till

https://hg.mozilla.org/integration/mozilla-inbound/rev/8299da5273e7c9748b6089a36b42868ecd9feb78
Bug 1263340 - Part 6: Use flags slot value instead of RegExp.prototype.flags in RegExpSplit. r=till

https://hg.mozilla.org/integration/mozilla-inbound/rev/ec86cd3cf0d3fee52ff0e37200dccf083f2a1fa7
Bug 1263340 - Part 7: Use internal slot for flags in RegExpReplace optimizable cases. r=till

https://hg.mozilla.org/integration/mozilla-inbound/rev/0beb66b9b2cc5d8bc9aae9561f1aa00f57399cdb
Bug 1263340 - Part 8: Separate RegExpSearch slow path. r=till

https://hg.mozilla.org/integration/mozilla-inbound/rev/1077b4771709b0df601d86e52f486d5e4345fb3f
Bug 1263340 - Part 9: Add optimizable path for RegExpMatch with global flag. r=till

Comment 43

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2d56da58f489
https://hg.mozilla.org/mozilla-central/rev/8299da5273e7
https://hg.mozilla.org/mozilla-central/rev/ec86cd3cf0d3
https://hg.mozilla.org/mozilla-central/rev/0beb66b9b2cc
https://hg.mozilla.org/mozilla-central/rev/1077b4771709