Bug 118150 – Use DeferrableOneShotTimer for deleting decoded data

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Antti Koivisto]

When loading apple.com/itunes CachedScript::script() takes ~1% of the web thread time, most of which is under system timer code.

We should use DeferrableOneShotTimer instead of plain Timer as that avoids unnecessary low level timer access.

[Antti Koivisto]

Created attachment 205640 [details]
patch

[Antti Koivisto]

<rdar://problem/14268416>

[Simon Fraser (smfr)]

Comment on attachment 205640 [details]
patch

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

> Source/WebCore/platform/Timer.h:158
> +    void setDelay(double delay) { m_delay = delay; }

setDelay() seems ambiguous here; does it reschedule the current timer? What does it mean to just change the delay like this?

[Antti Koivisto]

Created attachment 205648 [details]
patch

Remove the confusing delay setter, instead use a virtual function to vary the delay constructor argument.

[Andreas Kling]

Comment on attachment 205648 [details]
patch

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

r=me

> Source/WebCore/ChangeLog:35
> +            Avoid unnecessary restart after stopping and starting again.

Nice ninja fix.

[Simon Fraser (smfr)]

Comment on attachment 205648 [details]
patch

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

> Source/WebCore/loader/cache/CachedResource.cpp:894
> +double CachedResource::decodedDataDeletionTimerDelay() const
> +{
> +    return memoryCache()->deadDecodedDataDeletionInterval();
> +}

Shame that one is called an interval, and the other a delay.

[Antti Koivisto]

http://trac.webkit.org/changeset/152148

[Adam Barth]

Comment on attachment 205648 [details]
patch

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

> Source/WebCore/loader/cache/CachedScript.h:60
> +        virtual double decodedDataDeletionTimerDelay() const OVERRIDE;

Does this do what you mean?  You're calling decodedDataDeletionTimerDelay() in the CachedResource construct, which means you'll get the implementation from the base class because the CachedScript constructor hasn't run yet.

[Antti Koivisto]

Good point. It is indeed wrong.

[Antti Koivisto]

Created attachment 205803 [details]
don't call virtual in constructor

[Andreas Kling]

Comment on attachment 205803 [details]
don't call virtual in constructor

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

Oh wow, now I feel stupid. :|

> Source/WebCore/loader/cache/CachedResource.cpp:185
> +static double decodedDataDeletionTimerDelayForResourceType(CachedResource::Type type)

We should call this deadDecodedDataDeletionIntervalForResourceType() to match the MemoryCache function.

[Antti Koivisto]

http://trac.webkit.org/changeset/152219