Bug 98625

Summary: http://www.robohornet.org/tests/svgresize.html spends all its time in StyleResolver::sweepMatchedPropertiesCache
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarcelo, kling, koivisto, macpherson, menard, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 98798    
Attachments:
Description Flags
GC the matched properties cache on a timer instead of every 100 additions. none

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).