| Summary: | Avoid call to convertToASCIILowercase() in SelectorFilter::collectElementIdentifierHashes() in the common case | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||
| Component: | WebCore Misc. | Assignee: | Chris Dumez <cdumez> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | achristensen, darin, esprehn+autocc, ews-watchlist, ggaren, glenn, gyuyoung.kim, macpherson, menard, sam, webkit-bug-importer | ||||
| Priority: | P2 | Keywords: | InRadar | ||||
| Version: | WebKit Nightly Build | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Attachments: |
|
||||||
|
Description
Chris Dumez
2022-04-20 09:08:45 PDT
Created attachment 457993 [details]
Patch
Comment on attachment 457993 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=457993&action=review > Source/WebCore/css/SelectorFilter.cpp:50 > + AtomString tagLowercaseLocalName = LIKELY(element.isHTMLElement() && !element.isUnknownElement()) ? element.localName() : element.localName().convertToASCIILowercase(); A comment explaining this assumption would be useful even with the assert. Committed r293123 (249826@main): <https://commits.webkit.org/249826@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 457993 [details]. Comment on attachment 457993 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=457993&action=review >> Source/WebCore/css/SelectorFilter.cpp:50 >> + AtomString tagLowercaseLocalName = LIKELY(element.isHTMLElement() && !element.isUnknownElement()) ? element.localName() : element.localName().convertToASCIILowercase(); > > A comment explaining this assumption would be useful even with the assert. The classic way to do this is to write a function named localNameIsKnownToBeLowercase or localNameMayContainUppercaseASCII. Then that function is the perfect place for the comment explaining why the rule is correct, and here the code is more obviously correct. (In reply to Darin Adler from comment #5) > Comment on attachment 457993 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=457993&action=review > > >> Source/WebCore/css/SelectorFilter.cpp:50 > >> + AtomString tagLowercaseLocalName = LIKELY(element.isHTMLElement() && !element.isUnknownElement()) ? element.localName() : element.localName().convertToASCIILowercase(); > > > > A comment explaining this assumption would be useful even with the assert. > > The classic way to do this is to write a function named > localNameIsKnownToBeLowercase or localNameMayContainUppercaseASCII. Then > that function is the perfect place for the comment explaining why the rule > is correct, and here the code is more obviously correct. Oh, I missed Sam's comment before landing. Sorry about that. I'll follow-up. 0.38% progression on Speedometer on Intel, neutral on Apple Silicon. (In reply to Darin Adler from comment #5) > Comment on attachment 457993 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=457993&action=review > > >> Source/WebCore/css/SelectorFilter.cpp:50 > >> + AtomString tagLowercaseLocalName = LIKELY(element.isHTMLElement() && !element.isUnknownElement()) ? element.localName() : element.localName().convertToASCIILowercase(); > > > > A comment explaining this assumption would be useful even with the assert. > > The classic way to do this is to write a function named > localNameIsKnownToBeLowercase or localNameMayContainUppercaseASCII. Then > that function is the perfect place for the comment explaining why the rule > is correct, and here the code is more obviously correct. Done in <https://commits.webkit.org/r293138>. |