Bug 118150 - Use DeferrableOneShotTimer for deleting decoded data
Summary: Use DeferrableOneShotTimer for deleting decoded data
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-06-27 14:45 PDT by Antti Koivisto
Modified: 2013-07-01 06:28 PDT (History)
5 users (show)

See Also:


Attachments
patch (5.10 KB, patch)
2013-06-27 15:11 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (5.98 KB, patch)
2013-06-27 17:06 PDT, Antti Koivisto
kling: review+
Details | Formatted Diff | Diff
don't call virtual in constructor (4.18 KB, patch)
2013-07-01 06:10 PDT, Antti Koivisto
kling: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antti Koivisto 2013-06-27 14:45:26 PDT
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.
Comment 1 Antti Koivisto 2013-06-27 15:11:51 PDT
Created attachment 205640 [details]
patch
Comment 2 Antti Koivisto 2013-06-27 15:12:47 PDT
<rdar://problem/14268416>
Comment 3 Simon Fraser (smfr) 2013-06-27 16:07:15 PDT
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?
Comment 4 Antti Koivisto 2013-06-27 17:06:35 PDT
Created attachment 205648 [details]
patch

Remove the confusing delay setter, instead use a virtual function to vary the delay constructor argument.
Comment 5 Andreas Kling 2013-06-27 18:31:42 PDT
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.
Comment 6 Simon Fraser (smfr) 2013-06-27 18:36:28 PDT
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.
Comment 7 Antti Koivisto 2013-06-27 19:01:46 PDT
http://trac.webkit.org/changeset/152148
Comment 8 Adam Barth 2013-06-28 16:16:48 PDT
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.
Comment 9 Antti Koivisto 2013-07-01 05:29:24 PDT
Good point. It is indeed wrong.
Comment 10 Antti Koivisto 2013-07-01 06:10:22 PDT
Created attachment 205803 [details]
don't call virtual in constructor
Comment 11 Andreas Kling 2013-07-01 06:17:45 PDT
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.
Comment 12 Antti Koivisto 2013-07-01 06:28:38 PDT
http://trac.webkit.org/changeset/152219