Bug 238263 – Avoid unnecessary String constructor under FunctionExecutable::toStringSlow()

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

[Chris Dumez]

Avoid unnecessary String constructor under FunctionExecutable::toStringSlow().

[Chris Dumez]

Created attachment 455505 [details]
Patch

[Darin Adler]

Comment on attachment 455505 [details]
Patch

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

> Source/JavaScriptCore/runtime/FunctionExecutable.cpp:144
> -    String functionHeader;
> +    ASCIILiteral functionHeader = ASCIILiteral::null();

A few other thoughts: Why ASCIILiteral::null() here, but ""_s below? I’m not familiar with the variadic jsMakeNontrivialString. Does jsMakeNontrivialString benefit from this being an ASCIILiteral rather than a const char* or a StringView? Since src is a StringView I get the impression that we don’t need to make a String so not sure why ASCIILiteral is better than const char*. I’m sort of surprised that this idiom is jsMakeNontrivialString instead of just jsNontrivialString(makeString()).

I think I would have done:

    auto functionHeader = "";

[Chris Dumez]

(In reply to Darin Adler from comment #2)
> Comment on attachment 455505 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=455505&action=review
> 
> > Source/JavaScriptCore/runtime/FunctionExecutable.cpp:144
> > -    String functionHeader;
> > +    ASCIILiteral functionHeader = ASCIILiteral::null();
> 
> A few other thoughts: Why ASCIILiteral::null() here, but ""_s below? I’m not
> familiar with the variadic jsMakeNontrivialString. Does
> jsMakeNontrivialString benefit from this being an ASCIILiteral rather than a
> const char* or a StringView? Since src is a StringView I get the impression
> that we don’t need to make a String so not sure why ASCIILiteral is better
> than const char*. I’m sort of surprised that this idiom is
> jsMakeNontrivialString instead of just jsNontrivialString(makeString()).
> 
> I think I would have done:
> 
>     auto functionHeader = "";

Technically, the initialization value doesn't matter much since the switch below handles all enum values. Previously, the initialization value was a null string, not an empty string. ASCIILiteral::null() thus seemed closer to pre-existing code than ""_s.

That said, I don't mind initializing to _""s.

jsMakeNontrivialString() doesn't benefit from using an ASCIILiteral here. Using an ASCIILiteral also doesn't hurt as far as I can tell. Using ASCIILiteral also could be helpful for perf if we ever decide to store the string length as a data member in ASCIILiteral.

I think it is best practice to use _s for all String literals in WebKit. It should always be as good or faster than "". If we have API that is slower with an ASCIILiteral, I think that API would need to be fixed.

[Chris Dumez]

Created attachment 455509 [details]
Patch

[EWS]

Committed r291755 (248786@main): <https://commits.webkit.org/248786@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 455509 [details].

[Radar WebKit Bug Importer]

<rdar://problem/90713418>