Bug 238911

Summary: Reduce number of StringView to String conversions in JSC
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: JavaScriptCoreAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cmarcelo, darin, ews-watchlist, ggaren, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Chris Dumez 2022-04-06 16:50:39 PDT
Reduce number of StringView to String conversions in JSC.
Comment 1 Chris Dumez 2022-04-06 17:05:47 PDT
Created attachment 456878 [details]
Patch
Comment 2 Chris Dumez 2022-04-06 18:15:23 PDT
Created attachment 456880 [details]
Patch
Comment 3 Darin Adler 2022-04-07 12:36:55 PDT
Comment on attachment 456880 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=456880&action=review

> Source/JavaScriptCore/dfg/DFGLazyJSValue.cpp:54
> +        return jsString(vm, String { u.stringImpl });

While this resolves ambiguity, it misses the chance to reduce reference count churn, since there’s no need for a temporary String here. I suppose we can fix that by adding an overload of jsString that takes an rvalue reference? Or one that takes a StringImpl*.

> Source/JavaScriptCore/dfg/DFGLazyJSValue.cpp:56
>          return jsString(vm, AtomStringImpl::add(u.stringImpl));

Same issue/opportunity here?

> Source/JavaScriptCore/jsc.cpp:1099
> +        return FileSystem::pathByAppendingComponent(cachePath, makeString(StringViewHashTranslator::hash(source()), '-', filename, ".bytecode-cache"));

Does this really need to be in the StringViewHashTranslator namespace? I know the hash translator has to use it, but is that the best way to call it?

> Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:211
> +    String rightHandSide = sourceText.substring(inIndex + inLength).toStringWithoutCopying().simplifyWhiteSpace();

What a waste creating a String here at all. Does this really need "simplifyWhitespace"? Maybe we can write a version that doesn’t allocate memory in the super-common case where there is no whitespace that needs to be simplified. Some day.

> Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:230
> +    String rightHandSide = sourceText.substring(instanceofIndex + instanceofLength).toStringWithoutCopying().simplifyWhiteSpace();

Ditto.

> Source/JavaScriptCore/runtime/IdentifierInlines.h:132
> +    return jsString(vm, String { identifier.impl() });

Same reference count churn thought as above.

> Source/JavaScriptCore/runtime/IdentifierInlines.h:139
> +    return jsString(vm, String { identifier.impl() });

Ditto.

> Source/JavaScriptCore/runtime/JSModuleLoader.cpp:235
> +    arguments.append(jsString(vm, String { moduleKey.impl() }));

Same reference count churn thing.

> Source/JavaScriptCore/runtime/JSModuleLoader.cpp:401
> +        result->putDirectIndex(globalObject, i++, jsString(vm, String { key.get() }));

Again.

> Source/JavaScriptCore/runtime/JSString.h:891
> +    auto impl = s.is8Bit() ? StringImpl::create(s.characters8(), s.length()) : StringImpl::create(s.characters16(), s.length());

Truly unfortunate if the StringView came from an existing String; I guess overloading decreases the chance that will occur.

> Source/JavaScriptCore/runtime/SymbolConstructor.cpp:128
> +    return JSValue::encode(jsString(vm, String { uid }));

Yet another chance to avoid reference count churn.
Comment 4 Chris Dumez 2022-04-07 14:26:14 PDT
Created attachment 456968 [details]
Patch
Comment 5 Chris Dumez 2022-04-07 19:49:00 PDT
Created attachment 457005 [details]
Patch
Comment 6 EWS 2022-04-08 10:56:03 PDT
Committed r292619 (249443@main): <https://commits.webkit.org/249443@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 457005 [details].
Comment 7 Radar WebKit Bug Importer 2022-04-08 10:57:18 PDT
<rdar://problem/91492071>