Skip to content
This repository has been archived by the owner on Jan 26, 2022. It is now read-only.

Reduce indirections and calls with side-effects to make it easier to achieve a reasonable baseline performance? #32

Closed
anba opened this issue Feb 1, 2018 · 4 comments

Comments

@anba
Copy link
Collaborator

anba commented Feb 1, 2018

I would have expected the following specifications for String.prototype.matchAll and RegExp.prototype [ @@matchAll ]:

String.prototype.matchAll ( regexp )

  1. Let O be ? RequireObjectCoercible(this value).
  2. If regexp is neither undefined nor null, then
    1. Let matcher be ? GetMethod(regexp, @@matchall).
    2. If matcher is not undefined, then
      1. Return ? Call(matcher, regexp, « O »).
  3. Let S be ? ToString(O).
  4. Let R be ? RegExpCreate(regexp, "g").
  5. Return ! CreateRegExpStringIterator(R, S, true, false).

RegExp.prototype [ @@matchall ] ( string )

  1. Let R be the this value.
  2. If Type(R) is not Object, throw a TypeError exception.
  3. Let S be ? ToString(string).
  4. Let C be ? SpeciesConstructor(R, %RegExp%).
  5. Let matcher be ? Construct(C, « R »).
  6. Let global be ? ToBoolean(? Get(matcher, "global")).
  7. Let fullUnicode be ? ToBoolean(? Get(matcher, "unicode").
  8. Let lastIndex be ? ToLength(? Get(R, "lastIndex")).
  9. Perform ? Set(matcher, "lastIndex", lastIndex, true).
  10. Return ! CreateRegExpStringIterator(matcher, S, global, fullUnicode).

String.prototype.matchAll:

  • Directly testing for @@matchAll avoids the extra IsRegExp call which can produce side-effects. And it also matches the calling pattern in all other String.prototype methods which take a RegExp argument.
  • For the non-RegExp case: Calling RegExpCreate to create a new RegExp object and then calling MatchAllIterator to create just another new RegExp seems wasteful and makes it harder to optimize this function for implementors.

RegExp.prototype [ @@matchall ]:

  • Similar to the case from above, avoiding the extra IsRegExp call makes it easier to optimize this case, because we don't have to worry about potential side-effects.
  • Calling the RegExp constructor without an explicit flags argument allows us to take the fast path in 21.2.3.1, step 4 (https://tc39.github.io/ecma262/#sec-regexp-pattern-flags).
@ljharb
Copy link
Member

ljharb commented Feb 1, 2018

Directly testing

Hmm - directly testing for Symbol.matchAll would mean that this method isn't testing if something is a regex; IsRegExp is the canonical way to do so. I do see what you mean about testing for only the specific symbol. @allenwb, @littledan, thoughts?

For the non-RegExp case: Calling RegExpCreate to create a new RegExp object and then calling MatchAllIterator to create just another new RegExp seems wasteful and makes it harder to optimize this function for implementors.

I could certainly pass the string into MatchAllIterator, and create the regex once - that seems like an easy optimization that would reduce observability yet work identically in the common case.

allows us to take the fast path

I explicitly and intentionally have prohibited that fast path, since the intention is to 100% hide observable lastIndex mutation; if the fast path is allowed, then the internal mutations to lastIndex will be observable on the provided RegExp object. (The goal is that the internal regex operations are entirely unobservable, hopefully to create an even faster path)

@ljharb
Copy link
Member

ljharb commented Feb 2, 2018

Ah, another reason I'm using isRegExp - I want delete RegExp.prototype[Symbol.matchAll] to not break the common case.

@ljharb
Copy link
Member

ljharb commented Feb 2, 2018

Filed #33 to reduce observability when a string is passed.

@ljharb
Copy link
Member

ljharb commented Mar 20, 2018

#33 handles some observability; moved IsRegExp discussions to #34.

@ljharb ljharb closed this as completed Mar 20, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants