[webkit-reviews] review granted: [Bug 238911] Reduce number of StringView to String conversions in JSC : [Attachment 456880] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 238911: Reduce number of StringView to String conversions in JSC
https://bugs.webkit.org/show_bug.cgi?id=238911

Attachment 456880: Patch

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




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 456880
  --> https://bugs.webkit.org/attachment.cgi?id=456880
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.