[JSC] RegExp.prototype's @@match and @@replace should use "flags" getter only #7281
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
582aab2 to
d5ae907
Compare
…ter only https://bugs.webkit.org/show_bug.cgi?id=248605 Reviewed by NOBODY (OOPS!). This change implements recent spec change [1], aligning RegExp.prototype's @@match / @@replace with @@split / @@matchall to use only "flags" getter to check for flags, which is observable. Ensures that DFG watches and constant-folds all related invoked prototype getters, which was proven to be neutral on both JetStream2 and Speedometer2. [1]: tc39/ecma262#2791 * JSTests/test262/expectations.yaml: Mark 12 test cases as passing. * Source/JavaScriptCore/builtins/BuiltinNames.h: * Source/JavaScriptCore/builtins/RegExpPrototype.js: (linkTimeConstant.hasObservableSideEffectsForRegExpMatch): (linkTimeConstant.matchSlow): (overriddenName.string_appeared_here.replace): (linkTimeConstant.hasObservableSideEffectsForRegExpSplit): * Source/JavaScriptCore/builtins/StringPrototype.js: (linkTimeConstant.hasObservableSideEffectsForStringReplace): * Source/JavaScriptCore/bytecode/LinkTimeConstant.h: * Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h: (JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects): * Source/JavaScriptCore/dfg/DFGFixupPhase.cpp: (JSC::DFG::FixupPhase::addStringReplacePrimordialChecks): * Source/JavaScriptCore/runtime/JSGlobalObject.cpp: (JSC::JSGlobalObject::init): * Source/JavaScriptCore/runtime/JSGlobalObject.h: * Source/JavaScriptCore/runtime/JSGlobalObjectInlines.h: (JSC::JSGlobalObject::regExpProtoFlagsGetter const): (JSC::JSGlobalObject::regExpProtoDotAllGetter const): (JSC::JSGlobalObject::regExpProtoHasIndicesGetter const): (JSC::JSGlobalObject::regExpProtoIgnoreCaseGetter const): (JSC::JSGlobalObject::regExpProtoMultilineGetter const): (JSC::JSGlobalObject::regExpProtoStickyGetter const):
d5ae907 to
64f2b05
Compare
|
EWS run on current version of this PR (hash 582aab2)
|
| @@ -291,11 +314,13 @@ function replace(strArg, replace) | |||
| if (!functionalReplace) | |||
| replace = @toString(replace); | |||
|
|
|||
| var global = regexp.global; | |||
| var flags = @toString(regexp.flags); | |||
There was a problem hiding this comment.
Is it used for the fast path? Or not used and only used for slow path? I think flags generates strings, and it is strictly worse than global's flag (just returning boolean) in terms of performance, so I would like to know that we have optimization for flags, or it is not used in the most places.
There was a problem hiding this comment.
It's used only for slow path of String.prototype.replace, if @hasObservableSideEffectsForStringReplace returns false.
webkit-commit-queue
pushed a commit
to sosukesuzuki/WebKit
that referenced
this pull request
Feb 21, 2025
…s` getter only https://bugs.webkit.org/show_bug.cgi?id=248605 Reviewed by Yusuke Suzuki. This patch implements recent spec change [1], aligning RegExp.prototype's @@match / @@replace with @@split / @@matchall to use only "flags" getter to check for flags, which is observable. This patch is based on the PR by Alexey Shvayka[2]. Also according to the PR, no performance regressions. [1]: tc39/ecma262#2791 [2]: WebKit#7281 * JSTests/test262/expectations.yaml: * Source/JavaScriptCore/builtins/RegExpPrototype.js: (linkTimeConstant.hasObservableSideEffectsForRegExpMatch): (linkTimeConstant.matchSlow): (overriddenName.string_appeared_here.replace): * Source/JavaScriptCore/builtins/StringPrototype.js: (linkTimeConstant.hasObservableSideEffectsForStringReplace): Canonical link: https://commits.webkit.org/290785@main
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
64f2b05
582aab2
🧪 style🛠 ios🛠 mac🛠 wpe🛠 🧪 win🛠 ios-sim🛠 mac-AS-debug🛠 gtk🛠 wincairo🧪 webkitperl🧪 ios-wk2🧪 api-mac🧪 gtk-wk2🧪 api-ios🧪 mac-wk1🧪 api-gtk🛠 🧪 jsc🛠 tv🧪 mac-wk2🛠 jsc-armv7🛠 tv-sim🧪 mac-AS-debug-wk2🧪 jsc-armv7-tests🛠 watch🧪 mac-wk2-stress🛠 jsc-mips🛠 watch-sim🧪 jsc-mips-tests