Add fast path to JSStringJoiner::join() when there is only one string in the vector #11970

cdumez · 2023-03-25T16:30:17Z

`47cdb9c`

Add fast path to JSStringJoiner::join() when there is only one string in the vector
https://bugs.webkit.org/show_bug.cgi?id=254463

Reviewed by Darin Adler.

* Source/JavaScriptCore/runtime/JSStringJoiner.cpp:
(JSC::JSStringJoiner::joinSlow):
(JSC::JSStringJoiner::join): Deleted.
* Source/JavaScriptCore/runtime/JSStringJoiner.h:
(JSC::JSStringJoiner::join):

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

96b098e

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🛠 gtk
	~~🧪 api-ios~~	✅ 🧪 mac-wk1	✅ 🧪 gtk-wk2
✅ 🛠 🧪 jsc	✅ 🛠 tv	✅ 🧪 mac-wk2	⏳ 🧪 api-gtk
✅ 🛠 🧪 jsc-arm64	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2	✅ 🛠 jsc-armv7
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress	✅ 🧪 jsc-armv7-tests
~~🛠 🧪 merge~~	✅ 🛠 watch-sim		✅ 🛠 jsc-mips
✅ 🛠 🧪 unsafe-merge			✅ 🧪 jsc-mips-tests

webkit-early-warning-system · 2023-03-25T16:30:36Z

EWS run on previous version of this PR (hash 960ebac)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🛠 gtk
	~~🧪 api-ios~~	✅ 🧪 mac-wk1	✅ 🧪 gtk-wk2
✅ 🛠 🧪 jsc	✅ 🛠 tv	✅ 🧪 mac-wk2	⏳ 🧪 api-gtk
✅ 🛠 🧪 jsc-arm64	✅ 🛠 tv-sim	~~🧪 mac-AS-debug-wk2~~	✅ 🛠 jsc-armv7
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress	✅ 🧪 jsc-armv7-tests
	✅ 🛠 watch-sim		✅ 🛠 jsc-mips
			✅ 🧪 jsc-mips-tests

cdumez · 2023-03-25T16:31:40Z

A/B testing says this is a 0.72% progression (98% confidence) on Speedometer on M1 and 0.7% progression on JetStream (98% confidence).

Jarred-Sumner · 2023-03-25T17:48:01Z

nice

darinadler · 2023-03-25T17:49:57Z

Source/JavaScriptCore/runtime/JSStringJoiner.h

+inline JSValue JSStringJoiner::join(JSGlobalObject* globalObject)
+{
+    if (m_strings.size() == 1)
+        return jsString(globalObject->vm(), m_strings[0].view.toString());


Why doesn't this use the underlying string if present? Seems that doing that could avoid allocating a new StringImpl in some cases. Maybe those aren't common enough.

I assumed the string view might point to only part of the underlying string? I am not very familiar with this code.

That’s correct. But:

if the lengths are equal, the underlying string can be used as is — I think this is a common case and the optimization of not allocating any memory for a StringImpl would be significant

if the string view’s length is smaller than the underlying string, we could use substringSharingImpl to create a new StringImpl that is small and shares the characters, but that may be a more difficult tradeoff since we could keep a large string alive, and may not be as common a case

I was really thinking of (1) above, and if it’s common enough it might lead to a bigger performance win.

Thanks for the clarification. I'll see if we can get a bigger win.

You are right that (1) is the common case. 99.1% of the cases in Speedometer where m_strings has a length of 1 in fact. I'll do some A/B testing on the bots.

Hmm, I tried the following patch (https://pastebin.com/Nwacq8mF) but A/B testing says this is no longer a progression. Weird.

webkit-early-warning-system · 2023-03-25T21:13:58Z

EWS run on current version of this PR (hash 96b098e)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🛠 gtk
	~~🧪 api-ios~~	✅ 🧪 mac-wk1	✅ 🧪 gtk-wk2
✅ 🛠 🧪 jsc	✅ 🛠 tv	✅ 🧪 mac-wk2	⏳ 🧪 api-gtk
✅ 🛠 🧪 jsc-arm64	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2	✅ 🛠 jsc-armv7
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress	✅ 🧪 jsc-armv7-tests
~~🛠 🧪 merge~~	✅ 🛠 watch-sim		✅ 🛠 jsc-mips
✅ 🛠 🧪 unsafe-merge			✅ 🧪 jsc-mips-tests

cdumez · 2023-03-26T18:19:53Z

Will land as-is for now since this patch tested as a progression. I'll look into refining in a follow-up.

… in the vector https://bugs.webkit.org/show_bug.cgi?id=254463 Reviewed by Darin Adler. * Source/JavaScriptCore/runtime/JSStringJoiner.cpp: (JSC::JSStringJoiner::joinSlow): (JSC::JSStringJoiner::join): Deleted. * Source/JavaScriptCore/runtime/JSStringJoiner.h: (JSC::JSStringJoiner::join): Canonical link: https://commits.webkit.org/262129@main

webkit-commit-queue · 2023-03-26T18:42:09Z

Committed 262129@main (47cdb9c): https://commits.webkit.org/262129@main

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

cdumez requested a review from a team as a code owner March 25, 2023 16:30

cdumez self-assigned this Mar 25, 2023

cdumez added the JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues. label Mar 25, 2023

cdumez requested review from MenloDorian, Constellation, darinadler and geoffreygaren March 25, 2023 16:30

darinadler approved these changes Mar 25, 2023

View reviewed changes

cdumez force-pushed the 254463_JSStringJointer_join branch from 960ebac to 96b098e Compare March 25, 2023 21:12

cdumez added the merge-queue Applied to send a pull request to merge-queue label Mar 26, 2023

cdumez added unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing and removed merge-queue Applied to send a pull request to merge-queue labels Mar 26, 2023

webkit-commit-queue force-pushed the 254463_JSStringJointer_join branch from 96b098e to 47cdb9c Compare March 26, 2023 18:42

webkit-commit-queue merged commit 47cdb9c into WebKit:main Mar 26, 2023

webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Mar 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add fast path to JSStringJoiner::join() when there is only one string in the vector #11970

Add fast path to JSStringJoiner::join() when there is only one string in the vector #11970

cdumez commented Mar 25, 2023 •

edited by webkit-early-warning-system

webkit-early-warning-system commented Mar 25, 2023 •

edited

cdumez commented Mar 25, 2023

Jarred-Sumner commented Mar 25, 2023

darinadler Mar 25, 2023

cdumez Mar 25, 2023

darinadler Mar 25, 2023

cdumez Mar 25, 2023

cdumez Mar 25, 2023

cdumez Mar 26, 2023

webkit-early-warning-system commented Mar 25, 2023 •

edited

cdumez commented Mar 26, 2023

webkit-commit-queue commented Mar 26, 2023

Add fast path to JSStringJoiner::join() when there is only one string in the vector #11970

Add fast path to JSStringJoiner::join() when there is only one string in the vector #11970

Conversation

cdumez commented Mar 25, 2023 • edited by webkit-early-warning-system

47cdb9c

webkit-early-warning-system commented Mar 25, 2023 • edited

cdumez commented Mar 25, 2023

Jarred-Sumner commented Mar 25, 2023

darinadler Mar 25, 2023

Choose a reason for hiding this comment

cdumez Mar 25, 2023

Choose a reason for hiding this comment

darinadler Mar 25, 2023

Choose a reason for hiding this comment

cdumez Mar 25, 2023

Choose a reason for hiding this comment

cdumez Mar 25, 2023

Choose a reason for hiding this comment

cdumez Mar 26, 2023

Choose a reason for hiding this comment

webkit-early-warning-system commented Mar 25, 2023 • edited

cdumez commented Mar 26, 2023

webkit-commit-queue commented Mar 26, 2023

cdumez commented Mar 25, 2023 •

edited by webkit-early-warning-system

`47cdb9c`

webkit-early-warning-system commented Mar 25, 2023 •

edited

webkit-early-warning-system commented Mar 25, 2023 •

edited