[Webkit-unassigned] [Bug 38221] New: Memory issues due to the changes in 36556

[bugzilla-daemon at webkit.org]

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

           Summary: Memory issues due to the changes in 36556
           Product: WebKit
           Version: 528+ (Nightly build)
          Platform: PC
        OS/Version: Mac OS X 10.5
            Status: NEW
          Severity: Normal
          Priority: P2
         Component: CSS
        AssignedTo: webkit-unassigned at lists.webkit.org
        ReportedBy: slewis at apple.com
            Blocks: 36556


See http://bugs.webkit.org/show_bug.cgi?id=36556 for the inital change and
discussion.  After this was checked in memory concerns were raised

Comment #23 From Maciej Stachowiak 2010-04-23 15:30:13 PST (-) [reply]
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 From Stephanie Lewis 2010-04-23 15:33:53 PST (-) [reply]
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 From Antti Koivisto 2010-04-23 18:02:39 PST (-) [reply]
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 From Antti Koivisto 2010-04-23 18:16:42 PST (-) [reply]
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 From Maciej Stachowiak 2010-04-24 16:04:50 PST (-) [reply]
(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 From Kenneth Rohde Christiansen 2010-04-24 19:05:20 PST (-) [reply]
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 From Darin Adler 2010-04-25 22:02:52 PST (-) [reply]
(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 From Stephanie Lewis 2010-04-26 21:03:43 PST (-) [reply]
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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.