Bug 98625 - http://www.robohornet.org/tests/svgresize.html spends all its time in StyleResolver::sweepMatchedPropertiesCache
Summary: http://www.robohornet.org/tests/svgresize.html spends all its time in StyleRe...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 98798
  Show dependency treegraph
 
Reported: 2012-10-07 15:19 PDT by Eric Seidel (no email)
Modified: 2012-10-16 17:05 PDT (History)
6 users (show)

See Also:


Attachments
GC the matched properties cache on a timer instead of every 100 additions. (4.58 KB, patch)
2012-10-15 11:33 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2012-10-07 15:19:00 PDT
http://www.robohornet.org/tests/svgresize.html spends all (90%) of its time in StyleResolver::sweepMatchedPropertiesCache

It must be hitting some pathological case?
Comment 1 Eric Seidel (no email) 2012-10-07 15:21:15 PDT
Does the cache grow without bound?
Comment 2 Antti Koivisto 2012-10-10 04:32:05 PDT
Maybe the sweep needs be done by a short timer instead of being synchronous.
Comment 3 Andreas Kling 2012-10-15 11:33:58 PDT
Created attachment 168748 [details]
GC the matched properties cache on a timer instead of every 100 additions.
Comment 4 Eric Seidel (no email) 2012-10-15 11:43:59 PDT
Comment on attachment 168748 [details]
GC the matched properties cache on a timer instead of every 100 additions.

Should we be capping the size of this cache?
Comment 5 Eric Seidel (no email) 2012-10-15 11:46:53 PDT
Comment on attachment 168748 [details]
GC the matched properties cache on a timer instead of every 100 additions.

I now see that I asked the same question earlier in this bug. :)  Anyway, the change itself looks fine.  I still have some mild concern about the seemingly unbounded nature of this cache. :)
Comment 6 Eric Seidel (no email) 2012-10-15 11:47:40 PDT
Perhaps we should add svgresize or similar to PerformanceTests to catch this?
Comment 7 Andreas Kling 2012-10-15 11:50:07 PDT
(In reply to comment #4)
> (From update of attachment 168748 [details])
> Should we be capping the size of this cache?

That sounds like the sane thing to do. Thoughts, Antti?

(In reply to comment #6)
> Perhaps we should add svgresize or similar to PerformanceTests to catch this?

Could we import the test suite wholesale? If so, that would be the best thing to do. It might have a negative impact on cycle times (though that'll certainly motivate us to make the tests run faster!)
Comment 8 Eric Seidel (no email) 2012-10-15 11:51:50 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > Perhaps we should add svgresize or similar to PerformanceTests to catch this?
> 
> Could we import the test suite wholesale? If so, that would be the best thing to do. It might have a negative impact on cycle times (though that'll certainly motivate us to make the tests run faster!)

Bug 97507 is about just that.  I think the current prevailing view is that the suite is in-flux and thus not worth importing.  shrug.
Comment 9 WebKit Review Bot 2012-10-15 17:36:41 PDT
Comment on attachment 168748 [details]
GC the matched properties cache on a timer instead of every 100 additions.

Clearing flags on attachment: 168748

Committed r131388: <http://trac.webkit.org/changeset/131388>
Comment 10 WebKit Review Bot 2012-10-15 17:36:45 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Eric Seidel (no email) 2012-10-16 17:05:29 PDT
Good times. :)  Now the test spends 50% of time in StyleResolver::canShareStyleWithElement (which has shown up hot in other samples before).