New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 2 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
HW: All
NextAction: 2019-03-31
OS: ----
Priority: 2
Type: FeatureRequest



Sign in to add a comment
link

Issue 8522: Make Symbol.match not be uniquely special

Reported by mathias@chromium.org, Nov 29 Project Member

Issue description

https://github.com/tc39/ecma262/pull/1318 just achieved consensus at today’s TC39 meeting.

As with any change, there is a web compatibility risk (although it seems small in this case). Do we need a use counter for the old code path?
 

Comment 1 by gsat...@chromium.org, Nov 29

Is there a performance problem with adding the use counter? AFAIK all of this in CSA and the use counter is a runtime call, but it should be ok if this is only in the throwing case (is it? I'm not familiar enough with the PR)

Comment 2 by ljh...@gmail.com, Nov 29

My assumption is that the use counter would be added inside the RegExp constructor:
 1. whenever Symbol.match was not undefined and the object does not have a [[RegExpData]] internal slot
 2, and another for whenever Symbol.match was undefined and the object has a [[RegExpData]] internal slot

Would this cause a performance regression? Since the common case will have the slot, and have a non-undefined Symbol.match, I'm hoping not.

Comment 3 by ljh...@gmail.com, Nov 29

* correction: s/not undefined/truthy and s/undefined/falsy in my above comment.

Note we'll also have to include in the first counter "someone adds a truthy Symbol.match to String.prototype"

Comment 4 by jgruber@chromium.org, Nov 29

Components: Regexp
Owner: jgruber@chromium.org

Comment 5 by jgruber@chromium.org, Nov 29

I agree the compatibility risk seems small.

There's 4 uses of IsRegExp in the spec:

* String.p.endsWith
* String.p.includes
* String.p.startsWith

These all check IsRegExp(searchString) and throw if so. Note that if searchString is a string, then IsRegExp will already return false at the first step: 'If Type(argument) is not Object, return false.' The proposed change seems fairly safe here.

* The RegExp constructor

http://www.ecma-international.org/ecma-262/#sec-regexp-pattern-flags

This one is interesting, it distinguishes between a 'real' regexp (with an [[RegExpMatcher]] internal slot) and an object s.t. IsRegExp(pattern) returns true. In the latter case, pattern.source and pattern.flags properties are considered.

Adding use counters here sgtm. The RegExp ctor should be rarely called, I don't expect important perf regressions.

Comment 7 by bugdroid1@chromium.org, Dec 6

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/a0858cf0cad01296e7da59072a5919d9c3e8d006

commit a0858cf0cad01296e7da59072a5919d9c3e8d006
Author: Jakob Gruber <jgruber@chromium.org>
Date: Thu Dec 06 08:15:37 2018

[regexp] Add use counters for IsRegExp

A spec change to simplify IsRegExp has been proposed:

https://github.com/tc39/ecma262/pull/1318

This CL adds use counters for cases in which the spec change would
alter behavior:

1. o[@@match] is trueish but o is not a JSRegExp
2. o[@@match] is falseish (but not undefined) and o is a JSRegExp

This is the V8 side of required changes.
The Chromium-side CL: https://crrev.com/c/1360730

Drive-by: TNodeify IsRegExp.

Tbr: yangguo@chromium.org
Bug: v8:8522
Change-Id: I3766e02977f256a80d0e59472d3bafa9c692af9e
Reviewed-on: https://chromium-review.googlesource.com/c/1360630
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Reviewed-by: Sathya Gunasekaran <gsathya@chromium.org>
Reviewed-by: Mathias Bynens <mathias@chromium.org>
Commit-Queue: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#58064}
[modify] https://crrev.com/a0858cf0cad01296e7da59072a5919d9c3e8d006/include/v8.h
[modify] https://crrev.com/a0858cf0cad01296e7da59072a5919d9c3e8d006/src/builtins/builtins-regexp-gen.cc
[modify] https://crrev.com/a0858cf0cad01296e7da59072a5919d9c3e8d006/src/builtins/builtins-regexp-gen.h
[modify] https://crrev.com/a0858cf0cad01296e7da59072a5919d9c3e8d006/src/regexp/regexp-utils.cc
[modify] https://crrev.com/a0858cf0cad01296e7da59072a5919d9c3e8d006/test/cctest/test-usecounters.cc

Comment 8 by jgruber@chromium.org, Dec 6

NextAction: 2019-03-31
Counters landed, setting the alarm clock for when this has reached stable.

Comment 9 by bugdroid1@chromium.org, Dec 11

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/66308504a8e28cd4e6e597842af202a342c71ffc

commit 66308504a8e28cd4e6e597842af202a342c71ffc
Author: Jakob Gruber <jgruber@chromium.org>
Date: Tue Dec 11 13:11:26 2018

Add use counters for IsRegExp

A spec change to simplify IsRegExp has been proposed:

https://github.com/tc39/ecma262/pull/1318

This CL adds use counters for cases in which the spec change would
alter behavior:

1. o[@@match] is trueish but o is not a JSRegExp
2. o[@@match] is falseish (but not undefined) and o is a JSRegExp

This is the Chromium side of required changes.
The V8-side CL: https://crrev.com/c/1360630

Bug: v8:8522
Change-Id: I2ec9b1a5f54f8e70a02e48f280a548871990aabd
Reviewed-on: https://chromium-review.googlesource.com/c/1360730
Reviewed-by: Nico Weber <thakis@chromium.org>
Reviewed-by: Marja Hölttä <marja@chromium.org>
Commit-Queue: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615507}
[modify] https://crrev.com/66308504a8e28cd4e6e597842af202a342c71ffc/third_party/blink/public/platform/web_feature.mojom
[modify] https://crrev.com/66308504a8e28cd4e6e597842af202a342c71ffc/third_party/blink/renderer/bindings/core/v8/use_counter_callback.cc
[modify] https://crrev.com/66308504a8e28cd4e6e597842af202a342c71ffc/tools/metrics/histograms/enums.xml

Sign in to add a comment