String to literal comparisons should use StringView instead of WTF::equal in Blink |
||
Issue descriptionMany places in the code compare Strings with "some literal string". In these cases, we use operator==(const String& a, const char* b), which calls into WTF::equal. WTF::equal will iterate through a and b, looking for matches until either reaching the end of a, or a null terminator in b. In cases where we have access to the string literal though, we should know at compile time the length of b, so by wrapping b in a StringView, we can immediately fail fast if the lengths of the string differ. This will save time in string comparisons which share a common prefix (like "http" and "https"), where a O(prefix_length) comparison resolves to O(1) for different length strings. Note that this relies on the fact that compilers will optimize out strlen in these cases (see issue 673376). A complication here is that for const chars* that are *not* literals, we *don't* want to convert the const char* to a StringView, which will call strlen and avoid some of the nice optimizations in WTF::equal. Thus, I propose we just do find+replace and make only literal comparisons use StringView in Blink, rather than doing something more fancy with type conversion. This is achieved by the following command for direct literals: git grep -l "== \"[^\"]*\"" -- "third_party/WebKit/Source/*.cpp" | xargs sed -i 's/== \("[^"]*"\)/== StringView(\1)/g' This would convert all inline literals, but more work would need to be done for char* constants.
,
Jan 4 2017
Switching all to StringView SGTM if you think it's alright. I was just nervous to do it because of WTF::equal's cleverness in avoiding it. Are you planning on writing a presubmit hook?
,
Jan 4 2017
Presubmit hook for what? I don't think we should wrap all the literals in StringView, operator== should take a StringView instead.
,
Jan 4 2017
Ah, good point. We can do that if we just add operator==(String, StringView) and vice versa.
,
Jan 6 2017
Here's a change that does this: https://codereview.chromium.org/2614123002 I had been trying to actually use StringView in the arguments in my previous attempts, but it produces ambiguity and breaks the isAtomic() optimization inside WTF::equal() which was super hard to work around and I kept getting stuck. Thinking about it today I realized we can cheat and leave the various operator== overloads and call equalStringView inside them. This lets the compiler inline the const char* through the operator== into the StringView constructor (implicitly created from the call to equalStringView) which should then run strlen at compile time.
,
Jan 6 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e827c1a786ea596461b2c76ba6dafffd40f1bec8 commit e827c1a786ea596461b2c76ba6dafffd40f1bec8 Author: esprehn <esprehn@chromium.org> Date: Fri Jan 06 10:45:48 2017 Use StringView for String and AtomicString operator==. This allows the compiler to compute the length of literal strings that are compared and then we can early abort comparing if the length is different. This does introduce a strlen call for non-literal strings, which we have very few of today. This patch fixes the web idl enum checking loop to use WTF::equal to avoid the strlen since that seemed like an easy way to remove lots of strlen calls. We can make other callers use a similar approach in the future if if we see strlen on profiles. By code auditing (with code search method finding) I noticed a few other places will now call strlen (ex. iframe sandbox and permissions attribute parsing) but this patch opts not to convert all places we now call strlen and instead go for the simple path of fixing those if we run into problems. This is a similar trade off to code elsewhere in Chromium that uses base::StringPiece which also puts a strlen call for non-literal strings. I also took this as an opportunity to remove the operator== methods which aren't used for things like LChar* (strangely UChar* was missing) and Vector<char>. BUG= 678266 Review-Url: https://codereview.chromium.org/2614123002 Cr-Commit-Position: refs/heads/master@{#441914} [modify] https://crrev.com/e827c1a786ea596461b2c76ba6dafffd40f1bec8/third_party/WebKit/Source/bindings/core/v8/V8Binding.cpp [modify] https://crrev.com/e827c1a786ea596461b2c76ba6dafffd40f1bec8/third_party/WebKit/Source/wtf/text/AtomicString.h [modify] https://crrev.com/e827c1a786ea596461b2c76ba6dafffd40f1bec8/third_party/WebKit/Source/wtf/text/WTFString.h
,
Jan 20 2017
I think this can be closed out. |
||
►
Sign in to add a comment |
||
Comment 1 by esprehn@chromium.org
, Jan 4 2017