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.
Created attachment 51554 [details] Patch
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
Created attachment 51634 [details] Patch
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 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 on attachment 51634 [details] Patch This test passes fine on my local machine. I'm going to give the commit bot another try.
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
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.
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.
(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 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 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.
(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?
(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.
Created attachment 51792 [details] Patch
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 on attachment 51792 [details] Patch r=me
Committed r56744: <http://trac.webkit.org/changeset/56744>
Please roll out http://trac.webkit.org/changeset/56744. At least the changes to the python were invalid.
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.
(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.
validatereviewer.py fixed in http://trac.webkit.org/changeset/56748
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?
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.
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.
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.
(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.
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.
(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.
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
Filed https://bugs.webkit.org/show_bug.cgi?id=38221 on the memory impact.