Bug 38221 – Memory issues due to the changes in 36556

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Stephanie Lewis]

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

[Antti Koivisto]

It seems to me the cache could be in the JSCSSPrimitiveValue wrapper. I don't see why it must be part of the value itself. Wrappers are already shared and exist only when the CSSOM is actually being used.

[Darin Adler]

(In reply to comment #1)
> It seems to me the cache could be in the JSCSSPrimitiveValue wrapper. I don't
> see why it must be part of the value itself. Wrappers are already shared and
> exist only when the CSSOM is actually being used.

That's a neat idea.

[Sam Weinig]

Created attachment 56022 [details]
patch

I took the approach of an external cache to reclaim the memory loss.

[Darin Adler]

Comment on attachment 56022 [details]
patch

What's the speed impact of this, Sam?

[Sam Weinig]

(In reply to comment #4)
> (From update of attachment 56022 [details])
> What's the speed impact of this, Sam?

I didn't see a difference on Peacekeeper.

[Sam Weinig]

Fixed in r59386.

[WebKit Review Bot]

http://trac.webkit.org/changeset/59386 might have broken Leopard Intel Debug (Tests)