Bug 239426 – Leverage StringView in more places

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

[Chris Dumez]

Leverage StringView in more places, to reduce the number of String allocations.

[Chris Dumez]

Created attachment 457760 [details]
Patch

[Chris Dumez]

Created attachment 457761 [details]
Patch

[Chris Dumez]

Created attachment 457762 [details]
Patch

[Chris Dumez]

Created attachment 457764 [details]
Patch

[Chris Dumez]

Created attachment 457767 [details]
Patch

[Sam Weinig]

Comment on attachment 457767 [details]
Patch

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

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:148
> +    stringValue = StringView(stringValue).stripWhiteSpace().convertToASCIILowercase();

Going through the convertToASCIILowercase() and seeing how many can be converted to either explicit equalIgnoringASCIICase() or case-ignore SortedArrayMap would be a good investment.

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:234
> +                purposeStringValue = StringView(purposeStringValue).stripWhiteSpace().convertToASCIILowercase();
>                  Vector<String> keywords = purposeStringValue.splitAllowingEmptyEntries(" ");

This could totally be done without any allocation using SortedArrayMap and StringView's functor based split.

> Source/WebCore/editing/Editor.cpp:3224
> +    String transposed = makeString(text[1], text[0]);

Unrelated to this change, but it seems like this is totally not going to work with all graphemes. I bet transposing (control-t) two emoji doesn't work. Yep.

> Source/WebCore/platform/network/CacheValidation.cpp:301
> +                double maxAge = directives[i].second.toDouble(ok);

Another good project for the future is replacing these out parameter based conversion functions with std:;optional returning ones.

> Source/WebCore/platform/network/MIMEHeader.cpp:125
> +    auto encoding = text.stripWhiteSpace();

Another good SortedArrayMap use case for the future.

> Source/WebCore/platform/sql/SQLiteDatabase.cpp:464
> +        if (!executeCommandSlow(makeString("DROP TABLE ", table)))

Because string concatenation and databases make me very uncomfortable, can this / should this use prepareStatement + '?' + bindText()? (not relevant to this change, but a question none-the-less).

> Source/WebCore/platform/xr/PlatformXR.h:120
> +    auto feature = string.stripWhiteSpace().convertToASCIILowercase();

This would be more efficient / idiomatic if we switch to use a case insensitive SortedArrayMap (for the future, not relevant to the kind of changes you are making).

[Chris Dumez]

(In reply to Sam Weinig from comment #6)
> Comment on attachment 457767 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=457767&action=review
> 
> > Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:148
> > +    stringValue = StringView(stringValue).stripWhiteSpace().convertToASCIILowercase();
> 
> Going through the convertToASCIILowercase() and seeing how many can be
> converted to either explicit equalIgnoringASCIICase() or case-ignore
> SortedArrayMap would be a good investment.
> 
> > Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:234
> > +                purposeStringValue = StringView(purposeStringValue).stripWhiteSpace().convertToASCIILowercase();
> >                  Vector<String> keywords = purposeStringValue.splitAllowingEmptyEntries(" ");
> 
> This could totally be done without any allocation using SortedArrayMap and
> StringView's functor based split.
> 
> > Source/WebCore/editing/Editor.cpp:3224
> > +    String transposed = makeString(text[1], text[0]);
> 
> Unrelated to this change, but it seems like this is totally not going to
> work with all graphemes. I bet transposing (control-t) two emoji doesn't
> work. Yep.
> 
> > Source/WebCore/platform/network/CacheValidation.cpp:301
> > +                double maxAge = directives[i].second.toDouble(ok);
> 
> Another good project for the future is replacing these out parameter based
> conversion functions with std:;optional returning ones.
> 
> > Source/WebCore/platform/network/MIMEHeader.cpp:125
> > +    auto encoding = text.stripWhiteSpace();
> 
> Another good SortedArrayMap use case for the future.
> 
> > Source/WebCore/platform/sql/SQLiteDatabase.cpp:464
> > +        if (!executeCommandSlow(makeString("DROP TABLE ", table)))
> 
> Because string concatenation and databases make me very uncomfortable, can
> this / should this use prepareStatement + '?' + bindText()? (not relevant to
> this change, but a question none-the-less).

As far as I know (and just double checked on google), it isn't possible to prepare a statement and bind a table name. It only works for parameters, not table names.

> > Source/WebCore/platform/xr/PlatformXR.h:120
> > +    auto feature = string.stripWhiteSpace().convertToASCIILowercase();
> 
> This would be more efficient / idiomatic if we switch to use a case
> insensitive SortedArrayMap (for the future, not relevant to the kind of
> changes you are making).

[Chris Dumez]

Created attachment 457780 [details]
Patch

[Chris Dumez]

Created attachment 457783 [details]
Patch

[Chris Dumez]

(In reply to Sam Weinig from comment #6)
> Comment on attachment 457767 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=457767&action=review
> 
> > Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:148
> > +    stringValue = StringView(stringValue).stripWhiteSpace().convertToASCIILowercase();
> 
> Going through the convertToASCIILowercase() and seeing how many can be
> converted to either explicit equalIgnoringASCIICase() or case-ignore
> SortedArrayMap would be a good investment.

I wish I understood how to use SortedArrayMap with string literal keys...
ComparableASCIILiteral / ComparableCaseFoldingASCIILiteral / ComparableLettersLiteral with no explanation of what each is for.

Which ones are case sensitive and which ones are not for e.g. Probably obvious to some people but not to me. 

I looked at the SortedArrayMap API test, hoping it would help me understand. Sadly, only one test is actually trying uppercase and it is with ComparableLettersLiteral and somehow it matches. So somehow ComparableLettersLiteral is case-insensitive, which I find surprising.
Then what is ComparableCaseFoldingASCIILiteral for?

As often with WebKit code, I wish there was a smidge of documentation :)

[EWS]

Committed r292951 (249716@main): <https://commits.webkit.org/249716@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/91878229>

[Carlos Alberto Lopez Perez]

Committed r292952 (249717@trunk): <https://commits.webkit.org/249717@trunk>

[Darin Adler]

(In reply to Chris Dumez from comment #10)
> I wish I understood how to use SortedArrayMap with string literal keys...
> ComparableASCIILiteral / ComparableCaseFoldingASCIILiteral /
> ComparableLettersLiteral with no explanation of what each is for.

I’m happy to write more documentation.

> Which ones are case sensitive and which ones are not for e.g. Probably
> obvious to some people but not to me. 

ComparableASCIILiteral is case sensitive
ComparableCaseFoldingASCIILiteral is case insensitive
ComparableLettersLiteral is case insensitive

We can also make new ones; there is nothing special about this.

> So somehow
> ComparableLettersLiteral is case-insensitive, which I find surprising.
> Then what is ComparableCaseFoldingASCIILiteral for?

This is like equalIgnoringASCIICase and equalLettersIgnoringASCIICase.

ComparableCaseFoldingASCIILiteral is case-insensitve

ComparableLettersLiteral is case insensitive and more efficient when the keys are all guaranteed to only include the ASCII characters we can compare with the high speed "just or with 0x20" approach. We can give it a longer more-descriptive name. I think your complaints mostly boil down to unhappiness with that name.

> As often with WebKit code, I wish there was a smidge of documentation :)

Happy to add it.

We can also make additional ComparableXXX classes that build whitespace skipping into the canonicalization and comparison functions.