Bug 74782 - Reduce the memory allocations of WebCore's cssPropertyName()
Summary: Reduce the memory allocations of WebCore's cssPropertyName()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Benjamin Poulain
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-16 23:59 PST by Benjamin Poulain
Modified: 2012-02-03 18:02 PST (History)
6 users (show)

See Also:


Attachments
Patch (6.95 KB, patch)
2011-12-17 00:02 PST, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (7.03 KB, patch)
2012-01-13 17:45 PST, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (7.03 KB, patch)
2012-01-13 17:59 PST, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (3.62 KB, patch)
2012-01-27 17:38 PST, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (4.07 KB, patch)
2012-01-27 18:32 PST, Benjamin Poulain
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Poulain 2011-12-16 23:59:57 PST
The function cssPropertyName() show up in Dromaeo and PeaceKeeper due to the allocation of the String. We could reduce that by creating a new string only in the cases where the propertyName Identifier is different from the CSS property.
Comment 1 Benjamin Poulain 2011-12-17 00:02:16 PST
Created attachment 119716 [details]
Patch
Comment 2 Benjamin Poulain 2011-12-17 00:02:51 PST
The style breaking is intended.
Comment 3 WebKit Review Bot 2011-12-17 00:04:42 PST
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.
Comment 4 Michael Saboff 2011-12-21 13:31:55 PST
Besides the stylebot complaint, the patch looks good to me.
Comment 5 Benjamin Poulain 2012-01-05 10:56:44 PST
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 6 Benjamin Poulain 2012-01-05 10:56:46 PST
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 7 Benjamin Poulain 2012-01-13 17:45:28 PST
Created attachment 122524 [details]
Patch
Comment 8 Benjamin Poulain 2012-01-13 17:46:13 PST
Fixed the style...
Comment 9 Benjamin Poulain 2012-01-13 17:59:59 PST
Created attachment 122526 [details]
Patch
Comment 10 Benjamin Poulain 2012-01-13 18:01:25 PST
I get around 1-2% improvement on Peacekeeper, hard to get good numbers on that damn benchmark :(
Comment 11 Eric Seidel (no email) 2012-01-13 19:35:21 PST
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.
Comment 12 Benjamin Poulain 2012-01-13 21:02:22 PST
(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.
Comment 13 Benjamin Poulain 2012-01-14 19:40:17 PST
(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 14 Geoffrey Garen 2012-01-16 13:04:23 PST
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;
....
Comment 15 Geoffrey Garen 2012-01-16 13:05:36 PST
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.
Comment 16 Geoffrey Garen 2012-01-16 13:10:18 PST
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.
Comment 17 Michael Saboff 2012-01-16 13:25:02 PST
(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.
Comment 18 Benjamin Poulain 2012-01-16 15:25:01 PST
> 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.
Comment 19 Geoffrey Garen 2012-01-16 17:10:49 PST
> > 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.
Comment 20 Benjamin Poulain 2012-01-25 15:05:17 PST
> 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.
Comment 21 Geoffrey Garen 2012-01-25 21:49:32 PST
There's no need to realloc: an 8bit characters buffer is strictly smaller than a 16bit characters buffer.
Comment 22 Benjamin Poulain 2012-01-27 17:38:15 PST
Created attachment 124406 [details]
Patch
Comment 23 Benjamin Poulain 2012-01-27 17:39:46 PST
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.
Comment 24 Benjamin Poulain 2012-01-27 18:32:38 PST
Created attachment 124420 [details]
Patch
Comment 25 Geoffrey Garen 2012-01-30 11:18:36 PST
Comment on attachment 124420 [details]
Patch

r=me
Comment 26 Benjamin Poulain 2012-02-03 18:02:34 PST
Committed r106724: <http://trac.webkit.org/changeset/106724>