Bug 136179 – JavaScriptCore: Use ASCIILiteral where possible

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

[Joseph Pecoraro]

I wrote a tool to automatically make some string related fixes:

  1. Use ASCIILiteral where WTFString::String(const char*) was getting called (explicitly or implicitly) with a string literal
  2. Convert single character string literals to character literals for makeString components, or StringBuilder components
  3. Use jsNontrivialString instead of jsString if given a string literal >= length 2

Most of the changes look like:

>   - call("foo")
>   + call(ASCIILiteral("foo"))

Pros:
  - More efficient string construction (no copy) and potential memory savings <http://trac.webkit.org/wiki/EfficientStrings>
  - Clarification of when a string literal is being turned into WTFString

Cons:
  - uglier / more verbose source code
  - if a low level method (e.g. WTFString::find) were to add a new version supporting a "const char*" we would have to search for and revert the find(ASCIILiteral(...)) versions
    - however, having said that, they will likely be much easier to grep for

Ultimately we might want to make WTFString::String(const char *) an explicit constructor.

I could see us not wanting to make all these changes (esp in WebCore), or using this as a hint that we should create new methods taking string literals. Like WTFString::find(const char*)/WTFString::contains(const char *).

[Joseph Pecoraro]

Created attachment 237007 [details]
[PATCH] Proposed Fix

This also has a couple manual changes, like removing the only uses of String::append and switching to makeString in the inspector generator which makes more sense.

[WebKit Commit Bot]

Attachment 237007 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/TypeSet.cpp:220:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/TypeSet.cpp:222:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/TypeSet.cpp:224:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/TypeSet.cpp:226:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/TypeSet.cpp:228:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/TypeSet.cpp:230:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 6 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Saboff]

Comment on attachment 237007 [details]
[PATCH] Proposed Fix

r=me.

Look into the style complaints.
Also, please benchmark the change to make sure that it doesn't slow anything down before landing.

[Darin Adler]

Comment on attachment 237007 [details]
[PATCH] Proposed Fix

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

I’d like to see the script you used to find these checked in even if it’s not perfectly easy to run. Or maybe it was some kind of technique other than a script?

If you removed all the uses of String::append, shouldn’t we also remove String::append and StringImpl::append?

> Source/JavaScriptCore/dfg/DFGFunctionWhitelist.cpp:110
> +    String nameAndHash = name + '#' + hash;
>      return m_entries.contains(nameAndHash);

I suggest we get rid of the local variable.

> Source/JavaScriptCore/dfg/DFGOperations.cpp:140
> +        vm.throwException(exec, createRangeError(exec, ASCIILiteral("Requested length is negative")));

For this function and the other create error functions, I kind of wish we’d overload it directly since it’s almost always used with a literal. Might be tricky, though, since it has to be a function template to extract the literal’s length.

> Source/JavaScriptCore/runtime/TypeProfiler.cpp:49
> +    dataLog("\t\t#Local#\n\t\t", location->m_instructionTypeSet->seenTypes().replace(ASCIILiteral("\n"), ASCIILiteral("\n\t\t")), "\n");

This seems like it might be a case where we’d rather have the overloads than the ASCIILiteral.

[Darin Adler]

(In reply to comment #0)
> Ultimately we might want to make WTFString::String(const char *) an explicit constructor.

Seems neat. How close to that are we right now?

> I could see us not wanting to make all these changes (esp in WebCore), or using this as a hint that we should create new methods taking string literals. Like WTFString::find(const char*)/WTFString::contains(const char *).

I do think we should treat it as a hint to make functions that work directly on ASCII literals. Something like find(const char*) means we need to call strlen at runtime, which I think we avoid with ASCIILiteral, although it makes the code a little bigger. And it has the tricky issue about how it treats non-ASCII characters. It’s more efficient if we can assume it’s all ASCII, less good but still good if we can treat it as Latin-1, and even more useful in more cases if it’s treated as UTF-8, and it’s not obvious which of the three versions we should provide. If we make one intended for literals it’s likely to be accidentally misused on a UTF-8 string.

[Joseph Pecoraro]

(In reply to comment #4)
> (From update of attachment 237007 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=237007&action=review
> 
> I’d like to see the script you used to find these checked in even if it’s not perfectly easy to run. Or maybe it was some kind of technique other than a script?

It is not a script. It is a tool built with clang tools and tooling libraries:
<http://clang.llvm.org/docs/index.html#using-clang-as-a-library>

Unfortunately it is not easy to run. It is built with clang and you will need to build WebKit with this clang (a bit of a challenge likely requiring a few sudo commands setting up symlinks on Mac).


> If you removed all the uses of String::append, shouldn’t we also remove String::append and StringImpl::append?

I only searched for and removed all the uses of String::append in JavaScriptCore. I did not search all of WebCore, which I am sure there are some.

[Joseph Pecoraro]

(In reply to comment #5)
> (In reply to comment #0)
> > Ultimately we might want to make WTFString::String(const char *) an explicit constructor.
> 
> Seems neat. How close to that are we right now?

The tool only checked files built on the Mac port, so I'm sure it misses conversions in other ports. But, I imagine if we address all of the const char* <-> WTFString implicit conversions caught by this we would be pretty close.

[Joseph Pecoraro]

Created attachment 237372 [details]
[PATCH] For Landing

I ran performance tests and there was no difference, as we expected.

[WebKit Commit Bot]

Attachment 237372 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/TypeSet.cpp:224:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/TypeSet.cpp:226:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/TypeSet.cpp:228:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/TypeSet.cpp:230:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/TypeSet.cpp:232:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/TypeSet.cpp:234:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 6 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joseph Pecoraro]

(In reply to comment #4)
> > Source/JavaScriptCore/dfg/DFGOperations.cpp:140
> > +        vm.throwException(exec, createRangeError(exec, ASCIILiteral("Requested length is negative")));
> 
> For this function and the other create error functions, I kind of wish we’d overload it directly since it’s almost always used with a literal. Might be tricky, though, since it has to be a function template to extract the literal’s length.

I think we should be able to do this for ASCIILiteral itself, and if we can we only have to do it in one place to get the benefit of avoiding an strlen(). Ultimately these error strings get turned into JSStrings, so there might be even more room for improvement (no need to have a WTF::String intermediary) but I think exceptions are rare enough it shouldn't matter.


> > Source/JavaScriptCore/runtime/TypeProfiler.cpp:49
> > +    dataLog("\t\t#Local#\n\t\t", location->m_instructionTypeSet->seenTypes().replace(ASCIILiteral("\n"), ASCIILiteral("\n\t\t")), "\n");
> 
> This seems like it might be a case where we’d rather have the overloads than the ASCIILiteral.

Agreed. I won't make these kind of changes.

[Joseph Pecoraro]

Created attachment 237373 [details]
[PATCH] For Landing

Addressed the style issues.

[WebKit Commit Bot]

Comment on attachment 237373 [details]
[PATCH] For Landing

Clearing flags on attachment: 237373

Committed r173120: <http://trac.webkit.org/changeset/173120>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.