Bug 238911 – Reduce number of StringView to String conversions in JSC

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Chris Dumez]

Reduce number of StringView to String conversions in JSC.

[Chris Dumez]

Created attachment 456878 [details]
Patch

[Chris Dumez]

Created attachment 456880 [details]
Patch

[Darin Adler]

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.

[Chris Dumez]

Created attachment 456968 [details]
Patch

[Chris Dumez]

Created attachment 457005 [details]
Patch

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/91492071>