Normative: when `regexp` arg is a string, minimize observable operations #33

ljharb · 2018-02-02T01:26:21Z

Per #32.

…ons. Per #32.

anba · 2018-02-06T16:05:55Z

spec.emu

+		1. Else,
+			1. Let _flags_ be `"g"`.
+			1. Let _matcher_ be ? RegExpCreate(_regexp_, _flags_).
+			1. If ? IsRegExp(_matcher_) is not *true*, throw a *TypeError* exception.


Why would you want to check IsRegExp after creating a regular expression object through RegExpCreate? I don't see any reason to care about programs which have deleted RegExp.prototype[@@match].

I do; anything that's possible in an engine is something I care about.

Are there any other places in the specification with similar logic? There are many weird things that you can do with RegExp subclassing, but they don't tend to all have guards against them.

cc @allenwb

Definitely! Here's all the RegExp-related places with fallback logic when the well-known symbol isn't callable:

https://tc39.github.io/ecma262/#sec-string.prototype.match

https://tc39.github.io/ecma262/#sec-string.prototype.split

https://tc39.github.io/ecma262/#sec-string.prototype.search

https://tc39.github.io/ecma262/#sec-isregexp

https://tc39.github.io/ecma262/#sec-string.prototype.replace

etc.

In other words, every single one of these methods has an identical fallback.

I don't see how those fallbacks are like this one. Those are used to determine whether to take the string path, not whether to throw a TypeError. The places I see IsRegExp used are more about throwing a TypeError when a RegExp is passed into a method intended for a string (and that's throwing if IsRegExp is true, not false),

anba · 2018-02-06T16:06:51Z

spec.emu

+			1. Let _global_ be *true*.
+			1. Let _fullUnicode_ be *false*.
+			1. Let _lastIndex_ be *0*.
+			1. Assert: ! Get(_matcher_, `"lastIndex"`) is *0*.


You can't assert no abrupt completion occurs here, unless you remove IsRegExp. 😄

can you elaborate on why not?

var RegExpPrototypeMatch = RegExp.prototype[Symbol.match]; Object.defineProperty(RegExp.prototype, Symbol.match, { // This getter function is called when the abstract operation IsRegExp is called. get() { this.lastIndex++; return RegExpPrototypeMatch; } });

In what way does that produce an abrupt completion? Or are you pointing out that at this point, a throwing getter could be defined on this for lastIndex?

Oh, sorry I'm dumb. Pretend that I've written "You can't assert lastIndex is 0".

(Because if RegExp.prototype[@@match] has been redefined into a getter, it could have modified lastIndex.)

ahh, right. that's fair.

Do you think this should throw if it's not zero? or do you instead think that the check should just be removed, and replaced with a runtime check (every time lastIndex is retrieved) to ensure that lastIndex is a valid index?

Do you think this should throw if it's not zero?

I think IsRegExp should be removed.

anba · 2018-02-06T16:08:32Z

spec.emu

+			1. Let _R_ be _regexp_.
+			1. Let _C_ be ? SpeciesConstructor(_R_, %RegExp%).
+			1. Let _flags_ be ? ToString(? Get(_R_, `"flags"`)).
+			1. Let _matcher_ be ? Construct(_C_, &laquo; _R_, _flags_ &raquo;).


As mentioned in #32, I'd prefer Let matcher be ? Construct(C, « R »). here.

as noted here, the fast path is explicitly and intentionally prohibited, so a more important fast path can be taken (avoiding observable creation of this regex entirely in the common case).

I don't understand the justification w.r.t lastIndex. Where does lastIndex come into play when 21.2.3.1, step 4 is taken?

Let C be ? SpeciesConstructor(R, %RegExp%).

Let flags be ? ToString(? Get(R, "flags")).

Let matcher be ? Construct(C, « R, flags »).

Let global be ? ToBoolean(? Get(matcher, "global")).

Let fullUnicode be ? ToBoolean(? Get(matcher, "unicode").

Needs the following guards to avoid creating a separate RegExp object:

C is the built-in RegExp constructor

R is a built-in RegExp object

R doesn't redefine built-in RegExp.prototype functionality

R.[[Prototype]] is the built-in RegExp.prototype object (Note: Ignoring subclassing for now.)

RegExp.prototype.flags is the built-in getter

Implied by RegExp.prototype.flags:

RegExp.prototype.ignoreCase is the built-in getter

RegExp.prototype.multiline is the built-in getter

RegExp.prototype.dotAll is the built-in getter

RegExp.prototype.sticky is the built-in getter

RegExp.prototype.global is the built-in getter

RegExp.prototype.unicode is the built-in getter

~~RegExp.prototype.source is the built-in getter~~ (Edited)

RegExp.prototype[@@ match] is the built-in function

If all those guards hold, we can directly access R.[[OriginalSource]] and R.[[OriginalFlags]] and save them in the RegExp String Iterator object. Then we also store R in the RegExp String Iterator object and for %RegExpStringIteratorPrototype%.next() we can reuse R for matching as long as the current R.[[OriginalSource]] and R.[[OriginalFlags]] match the original properties (that's why we need to store them in the RegExp String Iterator object!). When we reuse R in this case, the internal matching obviously needs to ensure that it doesn't modify the lastIndex property of R.

If we use the alternative:

Let C be ? SpeciesConstructor(R, %RegExp%).

Let matcher be ? Construct(C, « R »).

Let global be ? ToBoolean(? Get(matcher, "global")).

Let fullUnicode be ? ToBoolean(? Get(matcher, "unicode").

we need the following guards:

C is the built-in RegExp constructor

R is a built-in RegExp object

R doesn't redefine built-in RegExp.prototype functionality

R.[[Prototype]] is the built-in RegExp.prototype object (Note: Ignoring subclassing for now.)

RegExp.prototype.global is the built-in getter

RegExp.prototype.unicode is the built-in getter

RegExp.prototype[@@ match] is the built-in function

So the alternative avoids the guards for RegExp.prototype.{global,ignoreCase,multiline,dotAll,~~source~~ (Edited)}. Does this make any difference for implementors? Well, it depends on the actual implementation. For example for SpiderMonkey ~~we will only need to add a guard for RegExp.prototype.source to [1], (Edited)~~ the ~~other~~ getters are already checked. But I have no idea if it'll make any difference for other implementors, we'd need to ask them...

[1] https://searchfox.org/mozilla-central/rev/a539f8f7fe288d0b4d8d6a32510be20f52bb7a0f/js/src/builtin/RegExp.cpp#1614

In your second example, however, if after the iterator is created, any of those guards are violated, wouldn't you have to start down a much slower path than in the first example?

Updated the comment above to remove RegExp.prototype.source from the list of properties which need to guarded.

For whatever reason I've always read 21.2.3.1, step 4 to require flags to be undefined... 😒

So... 21.2.3.1, step 4 will basically always be taken in the fast path (where fast path means that R is a built-in RegExp object).

In your second example, however, if after the iterator is created, any of those guards are violated, wouldn't you have to start down a much slower path than in the first example?

No, I don't think so, because in both cases we create a fresh RegExp object (at least in the spec).

The important part for me was if we can take 21.2.3.1, step 4, but given that I've misread if-condition in 21.2.3.1, my original concern was moot and we can probably leave it as is for now. As soon as we get some implementation experience, we may want to look at it again and see how good it can be optimized.

I may also propose some other changes for spec consistency reasons, but I haven't yet decided which ones make more sense. For example RegExp.prototype [ @@split ] derives its unicodeMatching variable from the retrieved flags instead of getting the unicode property. So we may want to do the same MatchAllIterator. Or RegExp.prototype [ @@match ] and RegExp.prototype [ @@replace ] only retrieve unicode when global is true, whereas MatchAllIterator always gets the unicode property, which is another slight inconsistency.

The retrieved flags are derived from checking each of the relevant properties: https://tc39.github.io/ecma262/#sec-get-regexp.prototype.flags

anba · 2018-02-06T16:14:47Z

spec.emu

-		1. Else,
-			1. Let _R_ be ? RegExpCreate(_regexp_, `"g"`).
-		1. Let _matcher_ be ? GetMethod(_R_, @@matchAll).
+		1. Let _matcher_ be ? GetMethod(_regexp_, @@matchAll).


String.prototype methods, including String.prototype.match, generally allow null/undefined as parameters and implicitly coerce them to the expected type. Why should String.prototype.matchAll differ here?

Good catch; this is missing a "neither undefined nor null, then" check.

anba · 2018-02-06T16:18:02Z

spec.emu


 	<p>The abstract operation _MatchAllIterator_ performs the following steps:</p>
 	<emu-alg>
-		1. If ? IsRegExp(_R_) is not *true*, throw a *TypeError* exception.
+		1. Perform ! RequireObjectCoercible(_regexp_).
+		1. If ? IsRegExp(_regexp_) is `true`, then


RegExp.prototype methods, including RegExp.prototype[@@match], generally throw a TypeError if the this-value is not an object. Why should RegExp.prototype[@@matchAll] differ here?

It doesn't? Perform ! RequireObjectCoercible(_regexp_). throws a TypeError if regexp is not an object.

It doesn't cover RegExp.prototype[@@matchAll].call(true, "") which should throw a TypeError, because true is not an object.

anba · 2018-02-06T16:22:12Z

spec.emu

+			1. Let _global_ be *true*.
+			1. Let _fullUnicode_ be *false*.
+			1. Let _lastIndex_ be *0*.
+			1. Assert: ! Get(_matcher_, `"lastIndex"`) is *0*.
 		1. Let _S_ be ? ToString(_O_).


This should be moved into String.prototype.matchAll resp. RegExp.prototype[@@matchAll] for consistency with other String.prototype resp. RegExp.prototype methods.

i'm not sure what you mean by "resp." here, can you elaborate?

ljharb · 2018-02-10T04:59:48Z

It sounds like #33 (comment) is the only remaining item on this PR; once that's resolved, it'd be great to get an approval so this can be merged :-)

Absolutely I'd love future issues to be filed if implementation experience indicates anything.

anba · 2018-02-12T16:50:31Z

"null".match(null) returns ["null"], but "null".matchAll(null) throws a TypeError with the current patch.

ljharb · 2018-02-26T21:14:08Z

@anba with the current state of the PR, 'null'.matchAll(null) should be identical to 'null'.matchAll('null'), no?

anba · 2018-02-27T19:09:40Z

String.prototype.matchAll(regexp)

Let O be ? RequireObjectCoercible(this value).
If regexp is neither undefined nor null, then
1. // not executed for regexp = null
Return ? MatchAllIterator(regexp, O).

MatchAllIterator ( regexp, O )

Perform ! RequireObjectCoercible(regexp). // <- Throws for regexp = null

ljharb · 2018-02-27T19:20:45Z

Ah, thanks, you're right. In this case that's a bug, because the ! is an assertion. Will fix.

…sh regex

anba · 2018-02-27T19:29:10Z

And RegExp.prototype [ @@matchAll ] ( string ) should start with:

Let rx be the this value.
If Type(rx) is not Object, throw a TypeError exception.
Return ? MatchAllIterator(rx, string).

ljharb · 2018-02-27T19:37:03Z

aha, thanks :-) that's why i'd added it originally. fixing that too.

ljharb · 2018-02-27T20:03:01Z

k; the PR is now updated; since the thread is quite long, can anyone (@littledan, @anba) post any remaining blockers for this PR - I would greatly appreciate further changes to be filed as separate issues, so I can consider those with a clean slate - as normal comments? (Linking to an existing review thread is totally great too)

anba · 2018-03-05T10:34:25Z

There are two nits in MatchAllIterator:

Let _R_ be _regexp_. could be removed and instead of R the regexp parameter can be used directly.
Let _lastIndex_ be *0*. can be removed, because lastIndex is unused in the else-branch.

Apart from that, I'd still like to see the IsRegExp call removed in the else-branch of MatchAllIterator.

And I'm not sure about the first IsRegExp call in MatchAllIterator and I wonder if it makes more sense to always take the first branch in MatchAllIterator if called from RegExp.prototype[@@matchAll] for consistency with RegExp.prototype[@@split]. But that should definitely be discussed in a different issue.

And finally, probably also for a different issue, the iterator calls CreateIterResultObject(null, true) instead of the normal CreateIterResultObject(undefined, true) when it is finished. I don't know if this is an intentional design decision or a bug?

anba · 2018-03-05T11:22:55Z

I've added some initial patches for SpiderMonkey at https://bugzilla.mozilla.org/show_bug.cgi?id=1435829.

Edit:
Performance-wise it looks promising, for example when I was running this µ-benchmark:

function f() {
    var s = "acbcbcab";
    // Simple RegExp so we don't measure the internal regular expression engine.
    var r = /a/g;
    var q = 0;
    var t = dateNow();
    for (var i = 0; i < 500000; ++i) {
        var m = s.match(r);
        if (m === null)
            continue;
        for (var x of m) {
            q += x.length;
        }
    }
    return [dateNow() - t, q];
}
for (var i = 0; i < 10; ++i) print(f());

Against the matchAll version:

function f() {
    var s = "acbcbcab";
    var r = /a/g;
    var q = 0;
    var t = dateNow();
    for (var i = 0; i < 500000; ++i) {
        for (var m of s.matchAll(r)) {
            q += m[0].length;
        }
    }
    return [dateNow() - t, q];
}
for (var i = 0; i < 10; ++i) print(f());

matchAll was only ~12.5% slower (in current SpiderMonkey tip with --nursery-strings=on). When more matches are present, e.g. with s = "acbcbcab".repeat(4), matchAll was even slightly faster (~5%).

The native matchAll implementation is also on a par with what I'd call a typical user-land implementation of String.prototype.matchAll. (This version intentionally doesn't follow the spec steps at all. It only provides a comparable interface, so we can compare it against the native matchAll implementation.):

class RegExpStringIter {
    constructor(regexp, string) {
        this.regexp = regexp;
        this.string = string;
        this.done = false;
    }
    [Symbol.iterator]() {
        return this;
    }
    next() {
        var value = null;
        if (!this.done) {
            value = this.regexp.exec(this.string);
            this.done = value === null || !this.regexp.global;
        }
        return {value, done: value === null};
    }
}

String.prototype.matchAll = function(regexp) {
    "use strict";
    return new RegExpStringIter(regexp, this);
};

Per #33 (comment)

ljharb · 2018-03-05T20:39:40Z

There are two nits in MatchAllIterator:

Done.

And I'm not sure about the first IsRegExp call in MatchAllIterator

#34

And finally, probably also for a different issue, the iterator calls CreateIterResultObject(null, true) instead of the normal CreateIterResultObject(undefined, true) when it is finished. I don't know if this is an intentional design decision or a bug?

Unintentional; I've fixed that in master.

Thanks! If that takes care of all the issues for this PR, I'd appreciate a PR review approval :-D

anba · 2018-03-05T21:13:50Z

Thanks! If that takes care of all the issues for this PR, I'd appreciate a PR review approval :-D

There's still that issue with the IsRegExp call in the else-branch of MatchAllIterator. Do we also want to move the discussion for that one to a different PR?

ljharb · 2018-03-05T23:14:27Z

@anba I filed #34 to discuss that; are you comfortable merging this as-is, and discussing that change there?

anba · 2018-03-06T12:36:15Z

#34 is about the first IsRegExp call, but the other IsRegExp call is still problematic, as discussed in #33 (comment) and #33 (comment).

Per #33 (comment)

ljharb · 2018-03-06T17:20:48Z

I've changed the incorrect lastIndex assertion per #33 (comment); could we move all "call IsRegExp less" discussion to #34? I'm happy to update its description.

…egExpStringIteratorPrototype% Tests were updated and assuming tc39/proposal-string-matchall#33 will be merged.

…matchall], and %RegExpStringIteratorPrototype% Tests were updated and assuming tc39/proposal-string-matchall#33 will be merged.

ljharb · 2018-03-20T11:45:12Z

Thanks for all the feedback so far! I'm going to merge this; let's continue further review in separate issues/PRs.

…matchall], and %RegExpStringIteratorPrototype% Tests were updated and assuming tc39/proposal-string-matchall#33 will be merged.

Per tc39/proposal-string-matchall#33 / tc39/proposal-string-matchall#35

…matchall], and %RegExpStringIteratorPrototype% Tests were updated and assuming tc39/proposal-string-matchall#33 will be merged.

Normative: when regexp arg is a string, minimize observable operati…

ad28a5a

…ons. Per #32.

ljharb mentioned this pull request Feb 2, 2018

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

Closed

anba reviewed Feb 6, 2018

View reviewed changes

ljharb added 2 commits February 7, 2018 00:12

Allow regexp to be null or undefined

e5c3009

Move observable ToString call to match match

d2775c3

Remove incorrect RequireObjectCoercible check; .match accepts a nulli…

610fc05

…sh regex

Ensure that the receiver of RegExp.prototype[@@matchall] is an Object

1c22edc

ljharb force-pushed the less_observability branch from 3547095 to 2197b6c Compare February 27, 2018 20:01

MatchAllIterator: Remove unneeded variables.

81ab6e7

Per #33 (comment)

ljharb mentioned this pull request Mar 5, 2018

Re-evaluate IsRegExp call in MatchAllIterator? #34

Closed

ljharb added a commit that referenced this pull request Mar 5, 2018

[Fix] use undefined in "done" CreateIterResultObject calls

f368659

Per #33 (comment)

ljharb force-pushed the less_observability branch from 2197b6c to 81ab6e7 Compare March 5, 2018 20:37

ljharb added spec text committee feedback labels Mar 5, 2018

[Fix] lastIndex might not be zero, even for a normal regex.

f49e428

Per #33 (comment)

peterwmwong added a commit to peterwmwong/test262 that referenced this pull request Mar 18, 2018

Fleshed out tests for String.p.matchAll, RegExp.p[@@matchall], and %R…

6a87be1

…egExpStringIteratorPrototype% Tests were updated and assuming tc39/proposal-string-matchall#33 will be merged.

peterwmwong mentioned this pull request Mar 19, 2018

Added/updated tests for matchAll ljharb/test262#1

Merged

ljharb merged commit 9c13a85 into master Mar 20, 2018

ljharb deleted the less_observability branch March 20, 2018 11:45

zloirock added a commit to zloirock/core-js that referenced this pull request Mar 21, 2018

update String#matchAll per tc39/proposal-string-matchall#33

e547ef5

zloirock added a commit to zloirock/core-js that referenced this pull request Mar 27, 2018

update String#matchAll per tc39/proposal-string-matchall#33

fb1d99e

zloirock added a commit to zloirock/core-js that referenced this pull request Mar 27, 2018

update String#matchAll per tc39/proposal-string-matchall#33

ea25afe

zloirock added a commit to zloirock/core-js that referenced this pull request Apr 3, 2018

update String#matchAll per tc39/proposal-string-matchall#33

a00c60b

zloirock added a commit to zloirock/core-js that referenced this pull request Apr 9, 2018

update String#matchAll per tc39/proposal-string-matchall#33

2fc583b

zloirock added a commit to zloirock/core-js that referenced this pull request Apr 21, 2018

update String#matchAll per tc39/proposal-string-matchall#33

d935e90

zloirock added a commit to zloirock/core-js that referenced this pull request May 4, 2018

update String#matchAll per tc39/proposal-string-matchall#33

a4a550d

zloirock added a commit to zloirock/core-js that referenced this pull request May 5, 2018

update String#matchAll per tc39/proposal-string-matchall#33

baf908a

zloirock added a commit to zloirock/core-js that referenced this pull request May 7, 2018

update String#matchAll per tc39/proposal-string-matchall#33

a66a049

zloirock added a commit to zloirock/core-js that referenced this pull request May 14, 2018

update String#matchAll per tc39/proposal-string-matchall#33

44390f7

zloirock added a commit to zloirock/core-js that referenced this pull request May 18, 2018

update String#matchAll per tc39/proposal-string-matchall#33

8cd2b7c

zloirock added a commit to zloirock/core-js that referenced this pull request May 26, 2018

update String#matchAll per tc39/proposal-string-matchall#33

31cbbe1

ljharb added a commit to es-shims/String.prototype.matchAll that referenced this pull request May 31, 2018

[Breaking] update to match latest spec

a7b7656

Per tc39/proposal-string-matchall#33 / tc39/proposal-string-matchall#35

ljharb added a commit to es-shims/String.prototype.matchAll that referenced this pull request May 31, 2018

[Breaking] update to match latest spec

92a6118

Per tc39/proposal-string-matchall#33 / tc39/proposal-string-matchall#35

mathiasbynens mentioned this pull request Sep 17, 2018

Committee feedback: fallback behavior #39

Closed

Normative: when regexp arg is a string, minimize observable operations #33

Normative: when regexp arg is a string, minimize observable operations #33

Conversation

ljharb commented Feb 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb Feb 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anba Feb 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anba Feb 9, 2018 • edited by ljharb Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb commented Feb 10, 2018

anba commented Feb 12, 2018

ljharb commented Feb 26, 2018

anba commented Feb 27, 2018

ljharb commented Feb 27, 2018

anba commented Feb 27, 2018

ljharb commented Feb 27, 2018

ljharb commented Feb 27, 2018

anba commented Mar 5, 2018

anba commented Mar 5, 2018 • edited Loading

ljharb commented Mar 5, 2018

anba commented Mar 5, 2018

ljharb commented Mar 5, 2018

anba commented Mar 6, 2018

ljharb commented Mar 6, 2018

ljharb commented Mar 20, 2018

Normative: when `regexp` arg is a string, minimize observable operations #33

Normative: when `regexp` arg is a string, minimize observable operations #33

ljharb Feb 7, 2018 •

edited

Loading

anba Feb 8, 2018 •

edited

Loading

anba Feb 9, 2018 •

edited by ljharb

Loading

anba commented Mar 5, 2018 •

edited

Loading