Summary: | Reduce the memory allocations of WebCore's cssPropertyName() | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Benjamin Poulain <benjamin> | ||||||||||||
Component: | WebCore Misc. | Assignee: | Benjamin Poulain <benjamin> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | darin, ddkilzer, eric, ggaren, msaboff, webkit.review.bot | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Benjamin Poulain
2011-12-16 23:59:57 PST
Created attachment 119716 [details]
Patch
The style breaking is intended. Attachment 119716 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:123: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
Total errors found: 1 in 2 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Besides the stylebot complaint, the patch looks good to me. Comment on attachment 119716 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=119716&action=review > Source/WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:99 > unsigned i = 0; Reading this again, I'll replace this by "unsigned prefixOffset = 0" and later use "!prefixOffset" instead of "i == 0". I'd still like a review for this version, I can fix that little style issue when landing if everything else is ok :) Comment on attachment 119716 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=119716&action=review > Source/WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:99 > unsigned i = 0; Reading this again, I'll replace this by "unsigned prefixOffset = 0" and later use "!prefixOffset" instead of "i == 0". I'd still like a review for this version, I can fix that little style issue when landing if everything else is ok :) Created attachment 122524 [details]
Patch
Fixed the style... Created attachment 122526 [details]
Patch
I get around 1-2% improvement on Peacekeeper, hard to get good numbers on that damn benchmark :( Could we write a more reliable micro-benchmark? We've certainly written such in the past. Trying to judge perf based on peacekeeper is folly. (In reply to comment #11) > Could we write a more reliable micro-benchmark? We've certainly written such in the past. Trying to judge perf based on peacekeeper is folly. Sure, I'll give it a shot. (In reply to comment #11) > Could we write a more reliable micro-benchmark? We've certainly written such in the past. Trying to judge perf based on peacekeeper is folly. I made this little benchmark: http://jsperf.com/css-property-access-by-name For the first case ("lowercaseCSSPropertyName"), we are 3 times as fast with the patch. For the 2 other cases, there is no regression. Interestingly, on my machine, Firefox is a lot faster than both Safari and Chrome for the first two cases. Comment on attachment 122526 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122526&action=review > Source/WebCore/ChangeLog:9 > + Previously, cssPropertyName() was always causing memory allocation > + if the Identifier is not null. Which identifier? > Source/WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:96 > +static String cssPropertyName(StringImpl* stringImpl, const CharType* characters, bool* hadPixelOrPosPrefix = 0) In the worst case ("epub-"), this function does 8 passes over stringImpl before taking the fast path. In the best case ("css-"), it does 2 passes. That might explain why Firefox is faster. You can get this down to one pass in all cases by writing out explicit branches like so: if (length < 3) return false; if (characters[0] == 'c') { if (characters[1] == 's') { if (characters[2] == 's') return true; .... It's odd to have a non-ASCII version of CSS property name matching: all CSS property names are ASCII. Instead of this: 156 if (stringImpl->is8Bit()) 157 return cssPropertyName(stringImpl, stringImpl->characters8(), hadPixelOrPosPrefix); 158 return cssPropertyName(stringImpl, stringImpl->characters16(), hadPixelOrPosPrefix); <followed by lots of function templates> I'd like to see something like this: if (!stringImpl->is8Bit()) if (!stringImpl->convertTo8Bit()) return String() return cssPropertyName(...) <followed by no function templates> StringImpl::convertTo8Bit doesn't need to malloc -- it can just perform a compaction on the existing 16bit buffer. Plus, it will be exceptionally rare. I'd also be curious to know what happens if you just use operator[] on the String, rather than the underlying characters buffer. That's a much simpler change, and it accomplishes 100% of your goal of removing the 16bit buffer malloc. (In reply to comment #16) > I'd also be curious to know what happens if you just use operator[] on the String, rather than the underlying characters buffer. That's a much simpler change, and it accomplishes 100% of your goal of removing the 16bit buffer malloc. operator[] does an internal is8Bit() check and then uses either the 8 or 16 bit buffer. operator equal is great for accessing one or two characters once. For multiple accesses like matching, access the underlying buffer via the appropriate bit-ness pointer. > I'd like to see something like this: > > if (!stringImpl->is8Bit()) > if (!stringImpl->convertTo8Bit()) > return String() > > return cssPropertyName(...) I had considered this, but I was afraid the Identifier would have to be converted back somewhere for a good reason. In which case we would have costly 8->16->8->16, etc. Maybe I am being overly pessimistic, I will test your suggestion. (In reply to comment #16) > I'd also be curious to know what happens if you just use operator[] on the String, rather than the underlying characters buffer. That's a much simpler change, and it accomplishes 100% of your goal of removing the 16bit buffer malloc. Michael answered about the operator[]. Note this patch also adds the path to skip the string builder altogether. This was really my goal here, the string conversion was a needed by-product. > > if (!stringImpl->is8Bit()) > > if (!stringImpl->convertTo8Bit()) > > return String() > > > > return cssPropertyName(...) > > I had considered this, but I was afraid the Identifier would have to be converted back somewhere for a good reason. In which case we would have costly 8->16->8->16, etc. Just to clarify, using my suggestion, it's possible to go 16->8->16+8, but no further conversions are possible afterwards. You can't ping-pong. > > I'd also be curious to know what happens if you just use operator[] on the String, rather than the underlying characters buffer. That's a much simpler change, and it accomplishes 100% of your goal of removing the 16bit buffer malloc. > > Michael answered about the operator[]. I'm not sure that Michael's comment rules out using operator[]. Yes, it branches. But we're only comparing a few characters here, so the branches may be OK. > I'd like to see something like this:
>
> if (!stringImpl->is8Bit())
> if (!stringImpl->convertTo8Bit())
> return String()
>
> return cssPropertyName(...)
>
> <followed by no function templates>
>
> StringImpl::convertTo8Bit doesn't need to malloc -- it can just perform a compaction on the existing 16bit buffer. Plus, it will be exceptionally rare.
I wanted to give that a shot, but it turns out there is no way to write "bool StringImpl::convertTo8Bit()" in a clean way. :(
StringImpl are allocated as a whole, not by allocating the character buffers. E.g.:
StringImpl* string = static_cast<StringImpl*>(fastMalloc(size));
This makes it impossible to realloc since it could change the StringImpl* pointer (and others can have references to the previous pointer).
Changing the Implementation of StringImpl to support convertTo8Bit() would cause performance regression since we would have to malloc twice.
There's no need to realloc: an 8bit characters buffer is strictly smaller than a 16bit characters buffer. Created attachment 124406 [details]
Patch
I have some ideas for the "slow case" to get those 2-3% back (or hopefully even faster than before). I will make followups for that. Created attachment 124420 [details]
Patch
Comment on attachment 124420 [details]
Patch
r=me
Committed r106724: <http://trac.webkit.org/changeset/106724> |