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 Rendering

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cmarcelo, kling, koivisto, macpherson, menard, webkit.review.bot

Priority:

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

Eric Seidel (no email)

Reported 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?

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
View All Add attachment proposed patch, testcase, etc.

Eric Seidel (no email)

Comment 1 2012-10-07 15:21:15 PDT

Does the cache grow without bound?

Antti Koivisto

Comment 2 2012-10-10 04:32:05 PDT

Maybe the sweep needs be done by a short timer instead of being synchronous.

Andreas Kling

Comment 3 2012-10-15 11:33:58 PDT

Created attachment 168748 [details] GC the matched properties cache on a timer instead of every 100 additions.

Eric Seidel (no email)

Comment 4 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?

Eric Seidel (no email)

Comment 5 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. :)

Eric Seidel (no email)

Comment 6 2012-10-15 11:47:40 PDT

Perhaps we should add svgresize or similar to PerformanceTests to catch this?

Andreas Kling

Comment 7 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!)

Eric Seidel (no email)

Comment 8 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.

WebKit Review Bot

Comment 9 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>

WebKit Review Bot

Comment 10 2012-10-15 17:36:45 PDT

All reviewed patches have been landed. Closing bug.

Eric Seidel (no email)

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

Note You need to log in before you can comment on or make changes to this bug.