Bug 74782 – Reduce the memory allocations of WebCore's cssPropertyName()

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Benjamin Poulain]

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.

[Benjamin Poulain]

Created attachment 119716 [details]
Patch

[Benjamin Poulain]

The style breaking is intended.

[WebKit Review Bot]

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.

[Michael Saboff]

Besides the stylebot complaint, the patch looks good to me.

[Benjamin Poulain]

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 :)

[Benjamin Poulain]

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 :)

[Benjamin Poulain]

Created attachment 122524 [details]
Patch

[Benjamin Poulain]

Fixed the style...

[Benjamin Poulain]

Created attachment 122526 [details]
Patch

[Benjamin Poulain]

I get around 1-2% improvement on Peacekeeper, hard to get good numbers on that damn benchmark :(

[Eric Seidel (no email)]

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.

[Benjamin Poulain]

(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.

[Benjamin Poulain]

(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.

[Geoffrey Garen]

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;
....

[Geoffrey Garen]

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.

[Geoffrey Garen]

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 Saboff]

(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.

[Benjamin Poulain]

> 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.

[Geoffrey Garen]

> > 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.

[Benjamin Poulain]

> 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.

[Geoffrey Garen]

There's no need to realloc: an 8bit characters buffer is strictly smaller than a 16bit characters buffer.

[Benjamin Poulain]

Created attachment 124406 [details]
Patch

[Benjamin Poulain]

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.

[Benjamin Poulain]

Created attachment 124420 [details]
Patch

[Geoffrey Garen]

Comment on attachment 124420 [details]
Patch

r=me

[Benjamin Poulain]

Committed r106724: <http://trac.webkit.org/changeset/106724>