Bug 104314 - Throw away StyleResolvers that haven't been used for a long time.
Summary: Throw away StyleResolvers that haven't been used for a long time.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-12-06 16:23 PST by Andreas Kling
Modified: 2012-12-08 01:03 PST (History)
6 users (show)

See Also:


Attachments
Proposed patch (4.72 KB, patch)
2012-12-06 16:31 PST, Andreas Kling
koivisto: review+
Details | Formatted Diff | Diff
Land crab (6.17 KB, patch)
2012-12-07 05:15 PST, 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 Andreas Kling 2012-12-06 16:23:14 PST
StyleResolver can get somewhat gigantic. Those that are not being used should be thrown away after some time since they can be reconstructed when needed.
Comment 1 Andreas Kling 2012-12-06 16:31:07 PST
Created attachment 178104 [details]
Proposed patch
Comment 2 Antti Koivisto 2012-12-06 16:53:04 PST
Comment on attachment 178104 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178104&action=review

> Source/WebCore/dom/Document.cpp:4446
> +    // Throttle the calls to startOneShot() a bit since it's non-trivial on some platforms.
> +    if (now > m_lastStyleResolverAccessTime + 5) {
> +        m_styleResolverThrowawayTimer.startOneShot(60);
> +        m_lastStyleResolverAccessTime = now;
> +    }

Please use named constants.
Comment 3 Andreas Kling 2012-12-07 05:15:20 PST
Created attachment 178201 [details]
Land crab
Comment 4 WebKit Review Bot 2012-12-07 08:33:15 PST
Comment on attachment 178201 [details]
Land crab

Clearing flags on attachment: 178201

Committed r136956: <http://trac.webkit.org/changeset/136956>
Comment 5 WebKit Review Bot 2012-12-07 08:33:19 PST
All reviewed patches have been landed.  Closing bug.
Comment 6 Ojan Vafai 2012-12-07 20:30:41 PST
Comment on attachment 178201 [details]
Land crab

View in context: https://bugs.webkit.org/attachment.cgi?id=178201&action=review

> Source/WebCore/dom/Document.cpp:4441
> +    static const int timeBeforeThrowingAwayStyleResolverAfterLastUseInSeconds = 60;
> +    static const int holdOffTimeBeforeReschedulingTimerInSeconds = 5;

Is it really important that these times be different? If not, you could make m_styleResolverThrowawayTimer a DeferrableOneShotTimer. Then this entire function would just be one line:
m_styleResolverThrowawayTimer.restart();
Comment 7 Andreas Kling 2012-12-08 01:03:10 PST
(In reply to comment #6)
> (From update of attachment 178201 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178201&action=review
> 
> > Source/WebCore/dom/Document.cpp:4441
> > +    static const int timeBeforeThrowingAwayStyleResolverAfterLastUseInSeconds = 60;
> > +    static const int holdOffTimeBeforeReschedulingTimerInSeconds = 5;
> 
> Is it really important that these times be different? If not, you could make m_styleResolverThrowawayTimer a DeferrableOneShotTimer. Then this entire function would just be one line:
> m_styleResolverThrowawayTimer.restart();

I was not aware of DeferrableOneShotTImer. I'll post a patch to use that instead, thanks!