Skip to content

[JSC] Do not resolve rope when constructing Function #47096

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jun 26, 2025
Merged

[JSC] Do not resolve rope when constructing Function #47096

merged 1 commit into from
Jun 26, 2025
+88 −26

Conversation

Constellation
Copy link
Member

@Constellation Constellation commented Jun 24, 2025 •
edited by webkit-early-warning-system
Loading

05cdfcb

[JSC] Do not resolve rope when constructing Function
https://bugs.webkit.org/show_bug.cgi?id=294878
rdar://154145026

Reviewed by Yijia Huang.

Let's avoid resolving string for input for Function constructor.
It is not so super efficient if we resolve them. We also avoid creating
SourceCode so that we do not need to allocate StringSourceProvider.

* Source/JavaScriptCore/runtime/FunctionConstructor.cpp:
(JSC::stringifyFunction):
(JSC::constructFunction):
(JSC::constructFunctionSkippingEvalEnabledCheck):
* Source/JavaScriptCore/runtime/FunctionExecutable.cpp:
(JSC::FunctionExecutable::fromGlobalCode):
* Source/JavaScriptCore/runtime/FunctionExecutable.h:
* Source/JavaScriptCore/runtime/JSGlobalObject.cpp:
(JSC::JSGlobalObject::tryGetCachedFunctionExecutableForFunctionConstructor):
* Source/JavaScriptCore/runtime/JSGlobalObject.h:
* Source/JavaScriptCore/runtime/JSString.h:
(WTF::StringTypeAdapter<JSC::JSString::StringTypeAdapter):
(WTF::StringTypeAdapter<JSC::JSString::length const):
(WTF::StringTypeAdapter<JSC::JSString::is8Bit const):
(WTF::StringTypeAdapter<JSC::JSString::writeTo const):
* Source/JavaScriptCore/runtime/JSStringInlines.h:
(JSC::JSString::resolveToBuffer):

Canonical link: https://commits.webkit.org/296661@main

9fd257f

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🛠 🧪 jsc ✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 🧪 jsc-arm64 ✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 🧪 unsafe-merge ✅ 🛠 tv ✅ 🛠 mac-safer-cpp ✅ 🛠 jsc-armv7
✅ 🛠 tv-sim ✅ 🧪 jsc-armv7-tests
✅ 🛠 watch
✅ 🛠 watch-sim

@Constellation Constellation requested a review from a team as a code owner June 24, 2025 02:26
@Constellation Constellation self-assigned this Jun 24, 2025
@Constellation Constellation added the JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues. label Jun 24, 2025
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 24, 2025 •
edited
Loading

EWS run on previous version of this PR (hash 34349b0)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ❌ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🛠 🧪 jsc ✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 🧪 jsc-arm64 ✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp ✅ 🛠 jsc-armv7
✅ 🛠 tv-sim ❌ 🧪 jsc-armv7-tests
✅ 🛠 watch
✅ 🛠 watch-sim

hyjorc1
hyjorc1 reviewed Jun 24, 2025
View reviewed changes
Source/JavaScriptCore/runtime/JSString.h Outdated
Comment on lines 1212 to 1211
void writeTo(std::span<CharacterType> destination) const
{
if (auto value = m_string->tryGetValueWithoutGC(); !value->isNull()) {
StringView { value }.getCharacters(destination);
return;
}
jsCast<JSC::JSRopeString*>(m_string)->resolveToBuffer(destination);
}
Copy link
Contributor

@hyjorc1 hyjorc1 Jun 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this correct? Are we sure that it's definitely a rope string after the first if check? And what happens if the value is null?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, right. Let me fix it.

@Constellation Constellation force-pushed the eng/JSC-Do-not-resolve-rope-when-constructing-Function branch from 34349b0 to d7f8b5d Compare June 25, 2025 01:46
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 25, 2025 •
edited
Loading

EWS run on previous version of this PR (hash d7f8b5d)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ❌ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🛠 🧪 jsc ✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 🧪 jsc-arm64 ✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ❌ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ❌ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp ✅ 🛠 jsc-armv7
✅ 🛠 tv-sim ❌ 🧪 jsc-armv7-tests
✅ 🛠 watch
✅ 🛠 watch-sim

@Constellation Constellation requested a review from hyjorc1 June 25, 2025 01:51
hyjorc1
hyjorc1 approved these changes Jun 25, 2025
View reviewed changes
Copy link
Contributor

@hyjorc1 hyjorc1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jun 25, 2025
@Constellation Constellation removed the merging-blocked Applied to prevent a change from being merged label Jun 25, 2025
@Constellation Constellation force-pushed the eng/JSC-Do-not-resolve-rope-when-constructing-Function branch from d7f8b5d to 9fd257f Compare June 25, 2025 15:53
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 25, 2025 •
edited
Loading

EWS run on current version of this PR (hash 9fd257f)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🛠 🧪 jsc ✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 🧪 jsc-arm64 ✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 🧪 unsafe-merge ✅ 🛠 tv ✅ 🛠 mac-safer-cpp ✅ 🛠 jsc-armv7
✅ 🛠 tv-sim ✅ 🧪 jsc-armv7-tests
✅ 🛠 watch
✅ 🛠 watch-sim

@Constellation Constellation added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Jun 26, 2025
@Constellation
[JSC] Do not resolve rope when constructing Function
05cdfcb 
https://bugs.webkit.org/show_bug.cgi?id=294878
rdar://154145026

Reviewed by Yijia Huang.

Let's avoid resolving string for input for Function constructor.
It is not so super efficient if we resolve them. We also avoid creating
SourceCode so that we do not need to allocate StringSourceProvider.

* Source/JavaScriptCore/runtime/FunctionConstructor.cpp:
(JSC::stringifyFunction):
(JSC::constructFunction):
(JSC::constructFunctionSkippingEvalEnabledCheck):
* Source/JavaScriptCore/runtime/FunctionExecutable.cpp:
(JSC::FunctionExecutable::fromGlobalCode):
* Source/JavaScriptCore/runtime/FunctionExecutable.h:
* Source/JavaScriptCore/runtime/JSGlobalObject.cpp:
(JSC::JSGlobalObject::tryGetCachedFunctionExecutableForFunctionConstructor):
* Source/JavaScriptCore/runtime/JSGlobalObject.h:
* Source/JavaScriptCore/runtime/JSString.h:
(WTF::StringTypeAdapter<JSC::JSString::StringTypeAdapter):
(WTF::StringTypeAdapter<JSC::JSString::length const):
(WTF::StringTypeAdapter<JSC::JSString::is8Bit const):
(WTF::StringTypeAdapter<JSC::JSString::writeTo const):
* Source/JavaScriptCore/runtime/JSStringInlines.h:
(JSC::JSString::resolveToBuffer):

Canonical link: https://commits.webkit.org/296661@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/JSC-Do-not-resolve-rope-when-constructing-Function branch from 9fd257f to 05cdfcb Compare June 26, 2025 13:55
@webkit-commit-queue
Copy link
Collaborator

Committed 296661@main (05cdfcb): https://commits.webkit.org/296661@main

Reviewed commits have been landed. Closing PR #47096 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit 05cdfcb into WebKit:main Jun 26, 2025
@webkit-commit-queue webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Jun 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hyjorc1 hyjorc1 hyjorc1 approved these changes

Assignees

@Constellation Constellation

Labels
JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Constellation @webkit-early-warning-system @webkit-commit-queue @hyjorc1 @webkit-ews-buildbot