Bug 104314

Summary:

Throw away StyleResolvers that haven't been used for a long time.

Product:

WebKit

Reporter:

Andreas Kling <kling>

Component:

CSS

Assignee:

Andreas Kling <kling>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

allan.jensen, cmarcelo, macpherson, menard, ojan.autocc, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Proposed patch	koivisto: review+
Land crab	none

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!