Bug 158891 – Optimize parseCacheHeader() by using StringView

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Chris Dumez]

Optimize parseCacheHeader() and avoid some temporary String allocations by using StringView.

[Chris Dumez]

Created attachment 281596 [details]
Patch

[Darin Adler]

Comment on attachment 281596 [details]
Patch

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

> Source/WTF/wtf/text/StringView.h:48
> +typedef bool (*CharacterMatchFunctionPtr)(UChar);

Not sure this type needs Ptr in its name.

In new code, we like to use using instead of typedef. Could we do that here?

> Source/WebCore/platform/network/CacheValidation.cpp:206
> +inline bool isControlCharacterOrWhitespace(UChar c)

I’d rename the argument to a word instead of a letter when touching this function.

> Source/WebCore/platform/network/CacheValidation.cpp:208
> +    return c < ' ' || c == 127 || isSpaceOrNewline(c);

If this really is about just the actual character space and the actual character newline, then I suggest just changing the "<" to "<=" instead of using a helper function. If this allows non-ASCII spaces, I have no idea why!

> Source/WebCore/platform/network/CacheValidation.cpp:211
> -inline String trimToNextSeparator(const String& str)
> +inline String trimToNextSeparator(const StringView& str)

Should be just StringView, not const StringView&. Might also rename it since we are touching this to be a word instead of a fragment "str".

Should return a StringView. I understand a conversion to string may eventually be necessary, but doing the trim operation alone does not require it.

> Source/WebCore/platform/network/CacheValidation.cpp:213
> -    return str.substring(0, str.find(isCacheHeaderSeparator));
> +    return str.substring(0, str.find(isCacheHeaderSeparator)).toString();

We lose the optimization here of reusing the string if there is no separator character. Which is a shame. Going over to StringView more completely makes that not an issue.

> Source/WebCore/platform/network/CacheValidation.cpp:216
>  static Vector<std::pair<String, String>> parseCacheHeader(const String& header)

This could be changed to just return a vector of pair<StringView, StringView>, since everything is within the header, right? That would be better.

> Source/WebCore/platform/network/CacheValidation.cpp:221
> +    const StringView safeHeader = safeHeaderString;

OK to use const, but a little strange. We don’t always use const on local variables just because we are not planning to change them.

[Chris Dumez]

Comment on attachment 281596 [details]
Patch

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

>> Source/WebCore/platform/network/CacheValidation.cpp:208
>> +    return c < ' ' || c == 127 || isSpaceOrNewline(c);
> 
> If this really is about just the actual character space and the actual character newline, then I suggest just changing the "<" to "<=" instead of using a helper function. If this allows non-ASCII spaces, I have no idea why!

I checked the RFC, we should indeed only consider ' ':
https://tools.ietf.org/html/rfc7234#appendix-B
https://tools.ietf.org/html/rfc5234#appendix-B.1

[Chris Dumez]

Comment on attachment 281596 [details]
Patch

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

>> Source/WebCore/platform/network/CacheValidation.cpp:216
>>  static Vector<std::pair<String, String>> parseCacheHeader(const String& header)
> 
> This could be changed to just return a vector of pair<StringView, StringView>, since everything is within the header, right? That would be better.

Actually, our StringViews point to the characters of 'safeHeaderString' internally, not 'header'. 'safeHeaderString' being a local variable, I don't think we can do this unfortunately.

[Chris Dumez]

Created attachment 281597 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 281597 [details]
Patch

Clearing flags on attachment: 281597

Committed r202188: <http://trac.webkit.org/changeset/202188>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 281597 [details]
Patch

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

> Source/WebCore/platform/network/CacheValidation.cpp:248
> +                result.append({ directive, trimToNextSeparator(value.substring(1, value.length() - 1)).toString() });

This could be written without the length() - 1, since that’s always implied in any call to substring.

    result.append({ directive, trimToNextSeparator(value.substring(1)).toString() });

[Chris Dumez]

(In reply to comment #8)
> Comment on attachment 281597 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=281597&action=review
> 
> > Source/WebCore/platform/network/CacheValidation.cpp:248
> > +                result.append({ directive, trimToNextSeparator(value.substring(1, value.length() - 1)).toString() });
> 
> This could be written without the length() - 1, since that’s always implied
> in any call to substring.
> 
>     result.append({ directive,
> trimToNextSeparator(value.substring(1)).toString() });

Indeed, fixed in <http://trac.webkit.org/changeset/202228>, thanks.