Closed Bug 1263340 Opened 8 years ago Closed 8 years ago

Change flags access in RegExpBuiltinExec from getter to internal slot.

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: arai, Assigned: arai)

References

Details

Attachments

(13 files, 1 obsolete file)

5.25 KB, patch
h4writer
: review+
Details | Diff | Splinter Review
3.44 KB, patch
h4writer
: review+
Details | Diff | Splinter Review
2.37 KB, patch
Details | Diff | Splinter Review
1.63 KB, patch
h4writer
: review+
Details | Diff | Splinter Review
2.79 KB, patch
h4writer
: review+
Details | Diff | Splinter Review
104.61 KB, patch
h4writer
: review+
Details | Diff | Splinter Review
1.74 KB, patch
till
: review+
Details | Diff | Splinter Review
168.54 KB, image/png
Details
4.23 KB, patch
till
: review+
Details | Diff | Splinter Review
7.08 KB, patch
till
: review+
Details | Diff | Splinter Review
7.63 KB, patch
till
: review+
Details | Diff | Splinter Review
1.83 KB, patch
till
: review+
Details | Diff | Splinter Review
4.14 KB, patch
till
: review+
Details | Diff | Splinter Review
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.
Assignee: nobody → arai.unmht
As a first step, changed global/sticky access to UnsafeGetInt32FromReservedSlot.
Attachment #8742335 - Flags: review?(till)
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?
Attachment #8742336 - Flags: feedback?(hv1989)
Also, now we don't use global/sticky accessor, so removed that check from RegExpPrototypeOptimizable.
Attachment #8742337 - Flags: review?(hv1989)
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 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)
Attachment #8742336 - Flags: feedback?(hv1989) → feedback+
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
Added code to MLoadFixedSlot::foldsTo, there object() is already a MRegExp instruction.
The performance result is almost same as before.
Attachment #8742336 - Attachment is obsolete: true
Attachment #8742373 - Flags: review?(hv1989)
Attachment #8742373 - Flags: review?(hv1989) → review+
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!
Attachment #8742337 - Flags: review?(hv1989) → review+
Attachment #8742335 - Flags: review?(till) → review?(hv1989)
See Also: → 1265992
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?
Attachment #8742335 - Flags: review?(hv1989) → review+
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.
Flags: needinfo?(hv1989)
(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
Flags: needinfo?(hv1989)
Waldo pointed out that RegExp's flags slot can be modified by RegExp#compile, so Part 1.5 is wrong...
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.
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.
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)
Attachment #8743672 - Flags: review?(hv1989)
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 ?
Attachment #8743672 - Flags: review?(hv1989) → review+
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.
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.
Keywords: leave-open
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
Attachment #8743672 - Attachment description: Part ?: Use internal slot for sticky flag in @@replace and @@search optimized path. → Part 2: Use internal slot for sticky flag in @@replace and @@search optimized path.
Attachment #8742337 - Attachment description: Part 2: Do not check global and sticky getters in RegExpPrototypeOptimizable. → Part -: Do not check global and sticky getters in RegExpPrototypeOptimizable.
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
Attachment #8744252 - Flags: review?(hv1989)
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") ?
Attachment #8744252 - Flags: review?(hv1989) → review+
See Also: → 1266764
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
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.
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.
Attachment #8751430 - Flags: review?(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?
Attachment #8751430 - Flags: review?(till) → review+
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
Blocks: 1266764
See Also: 1266764
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.
in addition to bug 1290414, added flags, so that we can skip flags getter call in RegExp#split.
Attachment #8804173 - Flags: review?(till)
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.
Attachment #8804174 - Flags: review?(till)
in @@replace optimized path, changed to use flags slot,
and moved related code to RegExpReplace function.

also moved rx.global to slow path.
Attachment #8804175 - Flags: review?(till)
@@search is already optimized, but splitting slow path into another function can improve performance for optimized case.
Attachment #8804176 - Flags: review?(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
Attachment #8804173 - Flags: review?(till) → review+
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.
Attachment #8804177 - Flags: review?(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|.
Attachment #8804174 - Flags: review?(till) → review+
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
Attachment #8804175 - Flags: review?(till) → review+
Comment on attachment 8804176 [details] [diff] [review]
Part 8: Separate RegExpSearch slow path.

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

nice! r=me
Attachment #8804176 - Flags: review?(till) → review+
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
Attachment #8804177 - Flags: review?(till) → review+
thank you for reviewing :D

clearing tracking flag, as this is performance thing, not wrong behavior etc.
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
Depends on: 1323108
You need to log in before you can comment on or make changes to this bug.