Bug 36556 – CSSPrimitiveValue::cssText() is slow

• Component: CSS
• Status: RESOLVED FIXED

[Stephen White]

CSSPrimitiveValue::cssText() (actually, the vsnprintf functions it calls) are showing up as hotspots in the Rendering test of the Peacekeeper benchmark.  In the "physics" demo of that test, CRT printf internals are consuming 37%+ of CPU on Chromium/Win.  There should be some low-hanging fruit there.

[Stephen White]

Created attachment 51554 [details]
Patch

[Darin Adler]

Comment on attachment 51554 [details]
Patch

Needs at least one test for each of the places where you had to clear m_cachedCSSText that would fail if you hadn't cleared out m_cachedCSSText. Can I comment out any one of the lines that set m_cachedCSSText and see a test fail?

It would be nice to make the caching work for FontFamilyValue, too.

r=me

[Stephen White]

Created attachment 51634 [details]
Patch

[Stephen White]

Ok, I've added a test for the normal case, and for setting the value via getPropertyCSSValue() / setFloatValue().

On further inspection, it turns out that init() is private, and only ever called from a constructor, so it's not necessary to reset the cached string in those cases.  I have removed that code.

[WebKit Commit Bot]

Comment on attachment 51634 [details]
Patch

Rejecting patch 51634 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 12543 test cases.
fast/css/font-face-multiple-remote-sources.html -> failed

Exiting early after 1 failures. 5245 tests run.
83.59s total testing time

5244 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
1 test case (<1%) had stderr output

Full output: http://webkit-commit-queue.appspot.com/results/1360006

[Stephen White]

Comment on attachment 51634 [details]
Patch

This test passes fine on my local machine.  I'm going to give the commit bot another try.

[WebKit Commit Bot]

Comment on attachment 51634 [details]
Patch

Rejecting patch 51634 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 12549 test cases.
fast/css/font-face-multiple-remote-sources.html -> failed

Exiting early after 1 failures. 5245 tests run.
82.34s total testing time

5244 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
1 test case (<1%) had stderr output

Full output: http://webkit-commit-queue.appspot.com/results/1352006

[Stephen White]

OK, this is getting ugly.  Turns out that, even without my changes, my new test causes a number of tests to fail, but only if the new test is run first.  Run individually, all the existing tests pass.

E.g.,
run-webkit-tests fast/css/cssText-cache.html -> pass
run-webkit-tests fast/css/font-smoothing.html -> pass
run-webkit-tests fast/css/cssText-cache.html fast/css/font-smoothing.html -> pass, fail

The bounding boxes in the output are about 2x larger than they should be, which causes the text diffs to fail.

I've narrowed it down to something in setFloatValue(), but I'm kind of at a loss at this point.  If I comment out that part of the test, everything is ok, but it'd be nice to track down the bug before this goes in.

[Darin Adler]

Something I have found useful in the many times I have dealt with problems like this is invoking DumpRenderTree directly, running it under GDB. So don't use the run-webkit-tests engine at all.

[Stephen White]

(In reply to comment #9)
> Something I have found useful in the many times I have dealt with problems like
> this is invoking DumpRenderTree directly, running it under GDB. So don't use
> the run-webkit-tests engine at all.

Thanks, yeah.  I've been doing that.  Nothing within the setFloatValue(), cleanup() or cssText() calls seems to be corrupt, or bad.  I'm guessing maybe it's corruption or some kind of odd side effect.

[Dave Hyatt]

Comment on attachment 51634 [details]
Patch

A number of cases in CSSPrimitiveValue::cssText() do early returns, so you need to refactor those so that they fall through to the end and actually use the cache.  rect(), attr(), pair all have this problem.

You yanked a FIXME that had useful information in it. Any reason, or just an accident?

[Dave Hyatt]

Comment on attachment 51634 [details]
Patch

setFloatValue is pretty buggy when used this way.  It doesn't really work right as a public DOM call.  It isn't working properly with full page zoom for example, and I also don't think it properly propagates changes to the front end RenderStyles either.

[Stephen White]

(In reply to comment #12)
> (From update of attachment 51634 [details])
> setFloatValue is pretty buggy when used this way.  It doesn't really work right
> as a public DOM call.  It isn't working properly with full page zoom for
> example, and I also don't think it properly propagates changes to the front end
> RenderStyles either.

Hmm, ok.  Darin had asked me to make sure the test exercised all code paths that clear the cache.  Is there another way to call setFloatValue() or setStringValue() from JS?  Or should I skip it for now, until the bugs are fixed?

[Stephen White]

(In reply to comment #11)
> (From update of attachment 51634 [details])
> A number of cases in CSSPrimitiveValue::cssText() do early returns, so you need
> to refactor those so that they fall through to the end and actually use the
> cache.  rect(), attr(), pair all have this problem.

Will fix.

> You yanked a FIXME that had useful information in it. Any reason, or just an
> accident?

I thought that the caching obviated the need to retain the original (unparsed) string, while also remaining spec-correct, but perhaps I misunderstood the comment.  I'll put it back.

I appreciate your (and Darin's) feedback, BTW.  This code is new to me and I want to make sure it's correct.

[Stephen White]

Created attachment 51792 [details]
Patch

[Stephen White]

OK, I restored the comment, fixed the early returns to enable caching, and took the setFloatValue() calls out of the test.  Let me know what you think.

[Dave Hyatt]

Comment on attachment 51792 [details]
Patch

r=me

[Stephen White]

Committed r56744: <http://trac.webkit.org/changeset/56744>

[Eric Seidel (no email)]

Please roll out http://trac.webkit.org/changeset/56744.  At least the changes to the python were invalid.

[Eric Seidel (no email)]

The python bits are the only parts that I'm suggesting are wrong.  You can pick on Adam Barth for making that check so draconian, however "Dave Hyatt" is not a reviewer according to committers.py.  "David Hyatt" is.

[Stephen White]

(In reply to comment #20)
> The python bits are the only parts that I'm suggesting are wrong.  You can pick
> on Adam Barth for making that check so draconian, however "Dave Hyatt" is not a
> reviewer according to committers.py.  "David Hyatt" is.

Sorry; I didn't mean to land that (I'm used to gcl semantics).  I couldn't make webkit-patch land it with either Dave or David in the ChangeLog.  Even if you specify it, it seems it still wants to pull the reviewer from the bug, and overrides what's in the ChangeLog.

Eric says he'll fix it.

[Eric Seidel (no email)]

validatereviewer.py fixed in http://trac.webkit.org/changeset/56748

[Maciej Stachowiak]

Stephanie, Geoff, this patch increases the size of CSSPrimitiveValue by a word. Do we have data to estimate the memory impact? How many CSSPrimitiveValues are created on a typical page? How many on membuster?

[Stephanie Lewis]

Any memory regression wasn't bad enough that I caught it among the other memory issues we're having, but I can take a look and get more concrete details.

[Antti Koivisto]

Lots of effort has went into reducing the memory consumption of the CSSOM and values:

https://bugs.webkit.org/show_bug.cgi?id=22379
https://bugs.webkit.org/show_bug.cgi?id=22717

We shouldn't regress without careful analysis, especially for a single benchmark. The only thing that stops this from being completely horrible is the sharing of CSSPrimitiveValues (which cuts down value object count by some 80%) . It will still be a significant hit with very large stylesheets.

There are alternative caching schemes (hash, rare value structure) that don't increase memory consumption. Have these been explored?

This patch also moves us to wrong direction by making it harder to turn css values into simple value type in the future.

[Antti Koivisto]

Generally, adding a cache field that is almost always null to an object that is instantiated in large numbers is basically never the right solution.

[Maciej Stachowiak]

(In reply to comment #24)
> Any memory regression wasn't bad enough that I caught it among the other memory
> issues we're having, but I can take a look and get more concrete details.

Just a count of CSSPrimitiveValue objects allocated on a typical page, or in the course of the membuster suite, would be useful.

[Kenneth Rohde Christiansen]

If we use a cache instead (hash for instance, as Antti suggested) we can control the size and thus the memory consumption. I think that would be preferred. It would also allow us to use different sizes on for instance desktop and mobile.

[Darin Adler]

(In reply to comment #28)
> If we use a cache instead (hash for instance, as Antti suggested) we can
> control the size and thus the memory consumption. I think that would be
> preferred. It would also allow us to use different sizes on for instance
> desktop and mobile.

I think that a "rare data" approach would be fine too; external storage in a hash table. I'm not sure that reclaiming memory when a script calls cssText on everything is important. What's much more important is avoiding extra memory cost in the normal, common case where cssText is not called.

We probably need a new bug report about this memory use regression rather than further discussion here in a bug already marked fixed.

[Stephanie Lewis]

Comparing Membuster data the patch was maybe a 1MB regression.  I can't really compare at that fine a level.

As far as instances go
8 CSSPrimitiveValue objects on an empty page
469 on Apple.com (+ ~40 every time the headline changes -- can climb pretty quickly)
460 on google.com
873 on nytimes.com

8626 on the first 30 pages on Membuster
87085 over the entire test

[Stephanie Lewis]

Filed https://bugs.webkit.org/show_bug.cgi?id=38221 on the memory impact.