Bug 36556 - CSSPrimitiveValue::cssText() is slow
Summary: CSSPrimitiveValue::cssText() is slow
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Normal
Assignee: Stephen White
URL: http://service.futuremark.com/peaceke...
Keywords:
Depends on: 38221
Blocks:
  Show dependency treegraph
 
Reported: 2010-03-24 14:18 PDT by Stephen White
Modified: 2010-04-27 14:44 PDT (History)
11 users (show)

See Also:


Attachments
Patch (3.35 KB, patch)
2010-03-24 15:05 PDT, Stephen White
no flags Details | Formatted Diff | Diff
Patch (4.93 KB, patch)
2010-03-25 07:01 PDT, Stephen White
no flags Details | Formatted Diff | Diff
Patch (5.56 KB, patch)
2010-03-26 16:07 PDT, Stephen White
hyatt: review+
senorblanco: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Stephen White 2010-03-24 14:18:18 PDT
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.
Comment 1 Stephen White 2010-03-24 15:05:40 PDT
Created attachment 51554 [details]
Patch
Comment 2 Darin Adler 2010-03-24 15:17:04 PDT
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
Comment 3 Stephen White 2010-03-25 07:01:06 PDT
Created attachment 51634 [details]
Patch
Comment 4 Stephen White 2010-03-25 07:04:11 PDT
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.
Comment 5 WebKit Commit Bot 2010-03-25 15:46:45 PDT
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
Comment 6 Stephen White 2010-03-25 18:17:12 PDT
Comment on attachment 51634 [details]
Patch

This test passes fine on my local machine.  I'm going to give the commit bot another try.
Comment 7 WebKit Commit Bot 2010-03-25 22:23:57 PDT
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
Comment 8 Stephen White 2010-03-26 12:57:07 PDT
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.
Comment 9 Darin Adler 2010-03-26 13:03:18 PDT
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.
Comment 10 Stephen White 2010-03-26 13:11:16 PDT
(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.
Comment 11 Dave Hyatt 2010-03-26 13:21:59 PDT
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?
Comment 12 Dave Hyatt 2010-03-26 13:28:50 PDT
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.
Comment 13 Stephen White 2010-03-26 14:46:26 PDT
(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?
Comment 14 Stephen White 2010-03-26 14:51:59 PDT
(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.
Comment 15 Stephen White 2010-03-26 16:07:09 PDT
Created attachment 51792 [details]
Patch
Comment 16 Stephen White 2010-03-27 12:53:15 PDT
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.
Comment 17 Dave Hyatt 2010-03-29 13:06:44 PDT
Comment on attachment 51792 [details]
Patch

r=me
Comment 18 Stephen White 2010-03-29 15:29:06 PDT
Committed r56744: <http://trac.webkit.org/changeset/56744>
Comment 19 Eric Seidel (no email) 2010-03-29 15:46:39 PDT
Please roll out http://trac.webkit.org/changeset/56744.  At least the changes to the python were invalid.
Comment 20 Eric Seidel (no email) 2010-03-29 15:51:08 PDT
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.
Comment 21 Stephen White 2010-03-29 16:03:01 PDT
(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.
Comment 22 Eric Seidel (no email) 2010-03-29 16:35:07 PDT
validatereviewer.py fixed in http://trac.webkit.org/changeset/56748
Comment 23 Maciej Stachowiak 2010-04-23 15:30:13 PDT
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?
Comment 24 Stephanie Lewis 2010-04-23 15:33:53 PDT
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.
Comment 25 Antti Koivisto 2010-04-23 18:02:39 PDT
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.
Comment 26 Antti Koivisto 2010-04-23 18:16:42 PDT
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.
Comment 27 Maciej Stachowiak 2010-04-24 16:04:50 PDT
(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.
Comment 28 Kenneth Rohde Christiansen 2010-04-24 19:05:20 PDT
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.
Comment 29 Darin Adler 2010-04-25 22:02:52 PDT
(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.
Comment 30 Stephanie Lewis 2010-04-26 21:03:43 PDT
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
Comment 31 Stephanie Lewis 2010-04-27 14:44:16 PDT
Filed https://bugs.webkit.org/show_bug.cgi?id=38221 on the memory impact.