Bug 98474 - Throttle DOM timers on hidden pages.
Summary: Throttle DOM timers on hidden pages.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kiran Muppala
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-10-04 19:01 PDT by Kiran Muppala
Modified: 2015-05-29 13:50 PDT (History)
29 users (show)

See Also:


Attachments
Patch (70.46 KB, patch)
2012-10-04 23:40 PDT, Kiran Muppala
no flags Details | Formatted Diff | Diff
Patch (71.74 KB, patch)
2012-10-05 02:29 PDT, Kiran Muppala
no flags Details | Formatted Diff | Diff
Patch (73.70 KB, patch)
2012-10-05 14:55 PDT, Kiran Muppala
no flags Details | Formatted Diff | Diff
Patch (73.13 KB, patch)
2012-10-06 16:08 PDT, Kiran Muppala
no flags Details | Formatted Diff | Diff
Patch (73.34 KB, patch)
2012-10-06 16:31 PDT, Kiran Muppala
no flags Details | Formatted Diff | Diff
Patch (73.48 KB, patch)
2012-10-08 20:15 PDT, Kiran Muppala
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kiran Muppala 2012-10-04 19:01:50 PDT
To minimize power usage, it will help to reduce the frequency at which DOM timers in background tabs fire.  For instance, limit them to no more than once a second.  Further, aligning the timer fire times to fall on regular intervals will minimize the number of CPU wakes and hence improve power.  Since, this feature could potentially break existing pages, it will be guarded by a feature define for further testing.
Comment 1 Kiran Muppala 2012-10-04 19:03:16 PDT
<rdar://problem/9692227>
Comment 2 Kiran Muppala 2012-10-04 20:27:00 PDT
To summarize, what this feature should do.  When a page visibility state changes to hidden, all it's DOM Timers should be updated so that their fire time is rounded to the nearest 1 second interval.  This aligning of timers limits the number of CPU wakes due to background tab timers to at most once every second.
Comment 3 Kiran Muppala 2012-10-04 22:11:13 PDT
Instead of rounding to the nearest alignment interval, which leads to issues with small timers as they can round down to lower than current time.  This could be fixed by setting a minimum at a fixed offset from current time, say minimumInterval().  But a cleaner solution is to just round up the fire time to the next multiple of alignment interval.  Hence, will be using this approach instead.
Comment 4 Kiran Muppala 2012-10-04 23:40:46 PDT
Created attachment 167261 [details]
Patch
Comment 5 Build Bot 2012-10-05 00:29:19 PDT
Comment on attachment 167261 [details]
Patch

Attachment 167261 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14176408
Comment 6 Kiran Muppala 2012-10-05 02:29:39 PDT
Created attachment 167289 [details]
Patch
Comment 7 Jesus Sanchez-Palencia 2012-10-05 07:12:28 PDT
Did you have a look at this? https://bugs.webkit.org/show_bug.cgi?id=85650
There are a few comments there that might come handy for you here.
Comment 8 mitz 2012-10-05 08:15:10 PDT
Even though browsers' background tabs are the motivation for this, I don't think we should use the word "tab" in the WebKit feature flag, since WebKit doesn't have tabs. Also, perhaps "throttling" and/or "coalescing" are better words to use than "reduction". 

Sorry that I don't have any comments on the code changes themselves, but I thought the terminology point was important enough to make.
Comment 9 Kiran Muppala 2012-10-05 10:42:55 PDT
(In reply to comment #7)
> Did you have a look at this? https://bugs.webkit.org/show_bug.cgi?id=85650
> There are a few comments there that might come handy for you here.

I wasn't aware of this.  Thanks.  Suspending animations i think is a good idea.  I will investigate if i can enable it on mac as well.  For timers though, suspending them completely may break more pages than just throttling them down.  So, this feature could be used in conjunction with suspending animations.
Comment 10 Kiran Muppala 2012-10-05 10:50:51 PDT
(In reply to comment #8)
> Even though browsers' background tabs are the motivation for this, I don't think we should use the word "tab" in the WebKit feature flag, since WebKit doesn't have tabs. Also, perhaps "throttling" and/or "coalescing" are better words to use than "reduction". 
> 
> Sorry that I don't have any comments on the code changes themselves, but I thought the terminology point was important enough to make.

Agreed, I will rename the feature to HIDDEN_PAGE_TIMER_THROTTLING.
Comment 11 Alexey Proskuryakov 2012-10-05 10:54:47 PDT
I'm not sure if this is something that should be behind a feature define. What kind of port-specific considerations against this behavior do you have in mind?

Even if this is to be enabled per-port, a runtime setting seems more appropriate on the surface. At least the test would not need to be skipped with a runtime setting.

How did you choose the one second interval? For example, do you have examples of pages that would break if timers were disabled completely? I feel like 1 second is a reasonable choice, but it would be good to know if specific research backs this number.

This should probably be checking if WebAudio is in use.
Comment 12 Simon Fraser (smfr) 2012-10-05 11:07:13 PDT
Comment on attachment 167289 [details]
Patch

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

> Source/WebCore/dom/ScriptExecutionContext.h:161
> +    void didChangeTimerAlignmentInterval();
> +    virtual double timerAlignmentInterval() const;

Why is ScriptExecutionContext the right place for this?

> Tools/ChangeLog:9
> +        Implement testRunner.setPageVisibility on mac for testing background tab
> +        timer reduction using DumpRenderTree.

I think it would be better to put it on window.internals
Comment 13 Kiran Muppala 2012-10-05 11:29:54 PDT
(In reply to comment #12)
> (From update of attachment 167289 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167289&action=review
> 
> > Source/WebCore/dom/ScriptExecutionContext.h:161
> > +    void didChangeTimerAlignmentInterval();
> > +    virtual double timerAlignmentInterval() const;
> 
> Why is ScriptExecutionContext the right place for this?
> 

Because DOMTimer has a reference to ScriptExecutionContext.

> > Tools/ChangeLog:9
> > +        Implement testRunner.setPageVisibility on mac for testing background tab
> > +        timer reduction using DumpRenderTree.
> 
> I think it would be better to put it on window.internals

The method was already declared on testRunner, although not implemented on mac.  Implementing it will allow page visibility tests to also use it when enabled.
Comment 14 Kiran Muppala 2012-10-05 14:55:33 PDT
Created attachment 167389 [details]
Patch
Comment 15 Simon Fraser (smfr) 2012-10-05 15:03:10 PDT
Comment on attachment 167389 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:2676
> +#if ENABLE(HIDDEN_PAGE_DOM_TIMER_THROTTLING)
> +double Document::timerAlignmentInterval() const
> +{
> +    Page* p = page();
> +    if (!p)
> +        return ScriptExecutionContext::timerAlignmentInterval();
> +    return p->settings()->domTimerAlignmentInterval();
> +}
> +#endif

Why does this and a lot of other code in this patch have to be inside the #ifdefs? Can't you just have timerAlignmentInterval() always return 0 when this feature is disabled?
Comment 16 Kiran Muppala 2012-10-05 15:05:01 PDT
(In reply to comment #11)
> I'm not sure if this is something that should be behind a feature define. What kind of port-specific considerations against this behavior do you have in mind?
> 
> Even if this is to be enabled per-port, a runtime setting seems more appropriate on the surface. At least the test would not need to be skipped with a runtime setting.
> 

My understanding is that the feature define is to allow for more livability testing, but what you said is also valid that, enabling on all platforms allows for more testing.  I have to check with Maciej to see what other reasons he had in mind for suggesting me to use a feature define.  In the interest of time, I think I will go ahead and check in my patch, since this does not affect the functionality of the feature in any way.  I will get back to you after I talk to Maciej.

> How did you choose the one second interval? For example, do you have examples of pages that would break if timers were disabled completely? I feel like 1 second is a reasonable choice, but it would be good to know if specific research backs this number.

The 1 second choice was initially a guess on a reasonable choice.  Testing showed that this performed very close to disabling timers all together.  So, decided to go with it.

> 
> This should probably be checking if WebAudio is in use.

The WebAudio API itself, I am told, does not rely on timers.  But if a page is poorly written, say using timers to load audio buffers, then there will be breaks in the audio.  I saw this behavior on http://chromium.googlecode.com/svn/trunk/samples/audio/shiny-drum-machine.html.  However the page could be written to use WebAudio events to load data and not run into this issue.
Comment 17 Kiran Muppala 2012-10-05 15:08:45 PDT
(In reply to comment #15)
> (From update of attachment 167389 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167389&action=review
> 
> > Source/WebCore/dom/Document.cpp:2676
> > +#if ENABLE(HIDDEN_PAGE_DOM_TIMER_THROTTLING)
> > +double Document::timerAlignmentInterval() const
> > +{
> > +    Page* p = page();
> > +    if (!p)
> > +        return ScriptExecutionContext::timerAlignmentInterval();
> > +    return p->settings()->domTimerAlignmentInterval();
> > +}
> > +#endif
> 
> Why does this and a lot of other code in this patch have to be inside the #ifdefs? Can't you just have timerAlignmentInterval() always return 0 when this feature is disabled?

The timer alignment code path needs an additional data member in TimerBase to keep track of the fire time without alignment.  I didn't want to add it unconditionally and use 0 for alignment interval.
Comment 18 Kiran Muppala 2012-10-05 15:10:37 PDT
(In reply to comment #14)
> Created an attachment (id=167389) [details]
> Patch

Renamed feature define to HIDDEN_PAGE_DOM_TIMER_THROTTLING.  Added a comment to DOMTimer.h to explain the effect of alignment interval and fixed initialization of visibility state when first creating a WebPageProxy.
Comment 19 Simon Fraser (smfr) 2012-10-05 15:21:26 PDT
(In reply to comment #17)
> (In reply to comment #15)
> > (From update of attachment 167389 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=167389&action=review
> > 
> > > Source/WebCore/dom/Document.cpp:2676
> > > +#if ENABLE(HIDDEN_PAGE_DOM_TIMER_THROTTLING)
> > > +double Document::timerAlignmentInterval() const
> > > +{
> > > +    Page* p = page();
> > > +    if (!p)
> > > +        return ScriptExecutionContext::timerAlignmentInterval();
> > > +    return p->settings()->domTimerAlignmentInterval();
> > > +}
> > > +#endif
> > 
> > Why does this and a lot of other code in this patch have to be inside the #ifdefs? Can't you just have timerAlignmentInterval() always return 0 when this feature is disabled?
> 
> The timer alignment code path needs an additional data member in TimerBase to keep track of the fire time without alignment.  I didn't want to add it unconditionally and use 0 for alignment interval.

So you can #ifdef that too, but in general we prefer to have most of the code un-ifdeffed.

Simon
Comment 20 Allan Sandfeld Jensen 2012-10-06 06:12:53 PDT
I wonder if it would make sense to integrate this with the suspend call. Essentially call suspend on the active DOM objects with a reason-for-suspend that indicates they are not currently visible but still in an active document (DocumentWillBeThrottled?). This way all the active DOM objects, not just timers can decide if they would like to throttle some of their events. It may sound like an abuse of the suspend system, but could help avoid any odd combinations of state by ensuring an active dom object is either suspended, throttled or fully active.
Comment 21 Kiran Muppala 2012-10-06 13:32:10 PDT
(In reply to comment #20)
> I wonder if it would make sense to integrate this with the suspend call. Essentially call suspend on the active DOM objects with a reason-for-suspend that indicates they are not currently visible but still in an active document (DocumentWillBeThrottled?). This way all the active DOM objects, not just timers can decide if they would like to throttle some of their events. It may sound like an abuse of the suspend system, but could help avoid any odd combinations of state by ensuring an active dom object is either suspended, throttled or fully active.

Suspension, i think, conveys that the timer would not fire until resumed.  And suspension correctly overrides throttling on DOM timers.  A suspended timer does not have any additional state about being throttled in the past.  Instead after being resumed, the timer re-evaluates if it needs to be throttled by looking at the alignment interval.  So, at any given time the timer is only in one of the states, suspended, throttled or fully active, as desired.
Comment 22 Adam Barth 2012-10-06 14:16:13 PDT
I know Chromium does this as well.  I wonder if it would make sense to unify the mechanisms.
Comment 23 Maciej Stachowiak 2012-10-06 14:21:11 PDT
I think there are two reasons to use a feature define for now:

(1) Aggressive background tab throttling is not yet fully baked. We know it has a risk of compatibility issues as coded presently. We ask other companies to put things behind a feature define when they are not yet stable, to give vendors the option to ship without it regardless of their schedule. A notable case where we asked for this was subpixel positioning. In fairness we should do the same.

(2) It's not clear that every port will want the more aggressive throtting. We would need to start a webkit-dev discussion to see if other port maintainers buy in. In my opinion we should start that discussion after the patch lands, so they can try it out for themselves.

So my suggestion is: land with the feature define, then start a discussion on webkit-dev inviting developers of other ports to consider whether they want this behavior and asking for help in testing it.
Comment 24 Maciej Stachowiak 2012-10-06 14:22:03 PDT
(In reply to comment #22)
> I know Chromium does this as well.  I wonder if it would make sense to unify the mechanisms.

This throttling is (I believe) much more aggressive than what Chromium does. I do think it would make sense to unify the mechanisms if Chromium folks agree that this level of throttling is appropriate.
Comment 25 Maciej Stachowiak 2012-10-06 14:25:47 PDT
Comment on attachment 167389 [details]
Patch

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

Looks good to me in general; I would suggest taking one more pass to see if it is practical to reduce the amount of code in ifdefs.

>>>> Source/WebCore/dom/Document.cpp:2676
>>>> +#endif
>>> 
>>> Why does this and a lot of other code in this patch have to be inside the #ifdefs? Can't you just have timerAlignmentInterval() always return 0 when this feature is disabled?
>> 
>> The timer alignment code path needs an additional data member in TimerBase to keep track of the fire time without alignment.  I didn't want to add it unconditionally and use 0 for alignment interval.
> 
> So you can #ifdef that too, but in general we prefer to have most of the code un-ifdeffed.
> 
> Simon

I agree that it makes sense to minimize the ifdefs. Even if some code has to be ifdef'd, it's still valuable to have as much code as possible shared.
Comment 26 Kiran Muppala 2012-10-06 14:30:20 PDT
(In reply to comment #24)
> (In reply to comment #22)
> > I know Chromium does this as well.  I wonder if it would make sense to unify the mechanisms.
> 
> This throttling is (I believe) much more aggressive than what Chromium does. I do think it would make sense to unify the mechanisms if Chromium folks agree that this level of throttling is appropriate.

Yes, the existing mechanism for throttling timers, namely setting the minimum timer interval, which is likely what Chromium uses, doesn't coalesce different timers together as aligning does.  Hence, the latter also leads to better power savings.
Comment 27 Maciej Stachowiak 2012-10-06 14:34:47 PDT
(In reply to comment #16)
> > 
> > This should probably be checking if WebAudio is in use.
> 
> The WebAudio API itself, I am told, does not rely on timers.  But if a page is poorly written, say using timers to load audio buffers, then there will be breaks in the audio.  I saw this behavior on http://chromium.googlecode.com/svn/trunk/samples/audio/shiny-drum-machine.html.  However the page could be written to use WebAudio events to load data and not run into this issue.

If existing WebAudio pages commonly have a problem with this level of throttling, then we should consider making WebAudio an exception. That doesn't have to be in the initial patch - we can live on it for a while and see what the fallout is.
Comment 28 Maciej Stachowiak 2012-10-06 14:42:57 PDT
(In reply to comment #26)
> (In reply to comment #24)
> > (In reply to comment #22)
> > > I know Chromium does this as well.  I wonder if it would make sense to unify the mechanisms.
> > 
> > This throttling is (I believe) much more aggressive than what Chromium does. I do think it would make sense to unify the mechanisms if Chromium folks agree that this level of throttling is appropriate.
> 
> Yes, the existing mechanism for throttling timers, namely setting the minimum timer interval, which is likely what Chromium uses, doesn't coalesce different timers together as aligning does.  Hence, the latter also leads to better power savings.

I think the complete set of differences is:

- This mechanism rounds fire time to an even multiple of a given time interval, while the mechanism used by Chromium applies a minimum to each timer interval individually (as described by Kiran above).

- This mechanism bypasses the "ramp-up" behavior of DOM timers, where the first 5 of a repeating timer or the first 5 of a chained series of one-shots get no clamping, while the mechanism used by Chromium still allows the initial ramp-up.

It does appear that Chromium's interval is one second, matching ours, so I may have overstated the case by saying "more aggressive". We'd be happy to share this mechanism with Chromium if you agree with the two policy choices above.
Comment 29 Maciej Stachowiak 2012-10-06 14:45:53 PDT
Comment on attachment 167389 [details]
Patch

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

> Source/WebCore/page/Page.cpp:1138
> +        setTimerAlignmentInterval(1); // Once a second

It may be better to use a named constant here instead of just a numeric literal 1. I realize that usually 1 is considered exempt from being a "magic number" but it literally is being used as one here. I'd suggest something like 

static const double hiddenPageTimerAlignmentInterval = 1.0
Comment 30 Maciej Stachowiak 2012-10-06 14:47:23 PDT
Comment on attachment 167389 [details]
Patch

Did not mean to mark "review+" just yet since I haven't closely reviewed all the code.
Comment 31 Maciej Stachowiak 2012-10-06 14:49:53 PDT
(In reply to comment #28)
> (In reply to comment #26)
> > (In reply to comment #24)
> > > (In reply to comment #22)
> > > > I know Chromium does this as well.  I wonder if it would make sense to unify the mechanisms.
> > > 
> > > This throttling is (I believe) much more aggressive than what Chromium does. I do think it would make sense to unify the mechanisms if Chromium folks agree that this level of throttling is appropriate.
> > 
> > Yes, the existing mechanism for throttling timers, namely setting the minimum timer interval, which is likely what Chromium uses, doesn't coalesce different timers together as aligning does.  Hence, the latter also leads to better power savings.
> 
> I think the complete set of differences is:
> 
> - This mechanism rounds fire time to an even multiple of a given time interval, while the mechanism used by Chromium applies a minimum to each timer interval individually (as described by Kiran above).
> 
> - This mechanism bypasses the "ramp-up" behavior of DOM timers, where the first 5 of a repeating timer or the first 5 of a chained series of one-shots get no clamping, while the mechanism used by Chromium still allows the initial ramp-up.
> 
> It does appear that Chromium's interval is one second, matching ours, so I may have overstated the case by saying "more aggressive". We'd be happy to share this mechanism with Chromium if you agree with the two policy choices above.

I'm starting to feel bad about spamming this bug, but:

- Another difference is that Chromium makes its own decision about what is eligible for timer throttling rather than relying on the same notion of visibility state used by the page visibility API.
Comment 32 Kiran Muppala 2012-10-06 16:08:08 PDT
Created attachment 167464 [details]
Patch
Comment 33 Kiran Muppala 2012-10-06 16:17:52 PDT
(In reply to comment #32)
> Created an attachment (id=167464) [details]
> Patch

Replaced magic number 1, with a static const as suggested by Maciej.

Eliminated most of the #ifdefs in WebCore and just used a default alignment interval of 0, as suggested by Simon.

In the process, identified a bug in TimerBase::setNextFire in my previous patch.  Namely, the unaligned fire time was not being updated if the requested fire time exactly matches the currently aligned fire time.  This has been fixed in this patch.
Comment 34 Kiran Muppala 2012-10-06 16:31:12 PDT
Created attachment 167465 [details]
Patch
Comment 35 Kiran Muppala 2012-10-06 16:32:42 PDT
(In reply to comment #34)
> Created an attachment (id=167465) [details]
> Patch

Fixed build error on chromium due to missing include <wtf/CurrentTime.h> in DOMTimer.cpp.
Updated date in ChangeLog entries.
Comment 36 Kiran Muppala 2012-10-08 13:28:52 PDT
(In reply to comment #10)
> (In reply to comment #8)
> > Even though browsers' background tabs are the motivation for this, I don't think we should use the word "tab" in the WebKit feature flag, since WebKit doesn't have tabs. Also, perhaps "throttling" and/or "coalescing" are better words to use than "reduction". 
> > 
> > Sorry that I don't have any comments on the code changes themselves, but I thought the terminology point was important enough to make.
> 
> Agreed, I will rename the feature to HIDDEN_PAGE_TIMER_THROTTLING.

After further discussion with Anders Carlsson and Dan Bernstein, the feature name was changed to HIDDEN_PAGE_DOM_TIMER_THROTTLING to be more descriptive of it's effect.
Comment 37 Maciej Stachowiak 2012-10-08 18:47:06 PDT
Comment on attachment 167465 [details]
Patch

Looks good to me and it looks like the prior comments have been addressed.
Comment 38 WebKit Review Bot 2012-10-08 18:53:48 PDT
Comment on attachment 167465 [details]
Patch

Rejecting attachment 167465 [details] from commit-queue.

cmuppala@apple.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.
Comment 39 WebKit Review Bot 2012-10-08 19:22:23 PDT
Comment on attachment 167465 [details]
Patch

Rejecting attachment 167465 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
X(target) out/Release/obj.target/webcore_dom/Source/WebCore/dom/ScriptRunner.o
Source/WebCore/dom/ScriptExecutionContext.cpp: In member function 'void WebCore::ScriptExecutionContext::didChangeTimerAlignmentInterval()':
Source/WebCore/dom/ScriptExecutionContext.cpp:402: error: 'struct WTF::KeyValuePair<int, WebCore::DOMTimer*>' has no member named 'second'
make: *** [out/Release/obj.target/webcore_dom/Source/WebCore/dom/ScriptExecutionContext.o] Error 1
make: *** Waiting for unfinished jobs....

Full output: http://queues.webkit.org/results/14218516
Comment 40 Kiran Muppala 2012-10-08 20:15:43 PDT
Created attachment 167675 [details]
Patch
Comment 41 Kiran Muppala 2012-10-08 20:24:19 PDT
(In reply to comment #40)
> Created an attachment (id=167675) [details]
> Patch

Fixed build error due to change in HashMap iterator interface, i.e. accessors first and second being renamed to key and value respectively.  Updated date in ChangeLog entries.
Comment 42 Maciej Stachowiak 2012-10-08 22:04:37 PDT
Comment on attachment 167675 [details]
Patch

r=me
Comment 43 WebKit Review Bot 2012-10-08 22:45:43 PDT
Comment on attachment 167675 [details]
Patch

Clearing flags on attachment: 167675

Committed r130720: <http://trac.webkit.org/changeset/130720>
Comment 44 WebKit Review Bot 2012-10-08 22:45:50 PDT
All reviewed patches have been landed.  Closing bug.
Comment 45 Kent Tamura 2012-10-08 22:52:03 PDT
Would you update http://trac.webkit.org/wiki/FeatureFlags please?
Comment 46 Kiran Muppala 2012-10-08 23:13:23 PDT
(In reply to comment #45)
> Would you update http://trac.webkit.org/wiki/FeatureFlags please?

Thanks for pointing me to the page.  Added HIDDEN_PAGE_DOM_TIMER_THROTTLING to the list.
Comment 47 Ojan Vafai 2012-10-09 14:57:37 PDT
Comment on attachment 167675 [details]
Patch

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

> LayoutTests/fast/dom/timer-throttling-hidden-page.html:8
> +	var previousT = new Date().getTime();

Could you fix up the indentation/style in this file and make the variable names more in line with WebKit style (e.g. s/previousT/previousTime)? It looks like you have tab characters in here. You should only use spaces, even for tests. We don't have an official style guide for tests, but they are something that we need to maintain, so deserve a minimum level of quality.
Comment 48 Ojan Vafai 2012-10-09 15:07:52 PDT
> - This mechanism rounds fire time to an even multiple of a given time interval, while the mechanism used by Chromium applies a minimum to each timer interval individually (as described by Kiran above).
> 
> - This mechanism bypasses the "ramp-up" behavior of DOM timers, where the first 5 of a repeating timer or the first 5 of a chained series of one-shots get no clamping, while the mechanism used by Chromium still allows the initial ramp-up.
> 
> It does appear that Chromium's interval is one second, matching ours, so I may have overstated the case by saying "more aggressive". We'd be happy to share this mechanism with Chromium if you agree with the two policy choices above.

I'm pretty sure Chromium would prefer the both of these policy choices. Adding jamesr to confirm.

> - Another difference is that Chromium makes its own decision about what is eligible for timer throttling rather than relying on the same notion of visibility state used by the page visibility API.

I think we do actually tie it to the page visibility API in practice. The code just lives elsewhere. At the very least, we throttle in superset of cases where the page is hidden as per the page visibility API, so having the code do this automatically would be fine with us.

I expect that in the end we could delete setMinimumTimerInterval from Page/Settings/DRT.
Comment 49 James Robinson 2012-10-09 15:20:45 PDT
(In reply to comment #48)
> > - This mechanism rounds fire time to an even multiple of a given time interval, while the mechanism used by Chromium applies a minimum to each timer interval individually (as described by Kiran above).
> > 
> > - This mechanism bypasses the "ramp-up" behavior of DOM timers, where the first 5 of a repeating timer or the first 5 of a chained series of one-shots get no clamping, while the mechanism used by Chromium still allows the initial ramp-up.
> > 
> > It does appear that Chromium's interval is one second, matching ours, so I may have overstated the case by saying "more aggressive". We'd be happy to share this mechanism with Chromium if you agree with the two policy choices above.
> 
> I'm pretty sure Chromium would prefer the both of these policy choices. Adding jamesr to confirm.

Those sound fine, assuming that web compat allows for it.

> 
> > - Another difference is that Chromium makes its own decision about what is eligible for timer throttling rather than relying on the same notion of visibility state used by the page visibility API.
> 
> I think we do actually tie it to the page visibility API in practice. The code just lives elsewhere. At the very least, we throttle in superset of cases where the page is hidden as per the page visibility API, so having the code do this automatically would be fine with us.
> 
> I expect that in the end we could delete setMinimumTimerInterval from Page/Settings/DRT.

That's correct - in chromium this behavior is tied to the same mechanism as the page visibility API.  Ken did this plumbing, I'm not sure if there is a particular reason for it being done outside of WebCore but it should work fine either way.
Comment 50 Kenneth Russell 2012-10-09 16:06:14 PDT
Comment on attachment 167675 [details]
Patch

As far as I can tell, this patch basically duplicates the functionality of DOMTimer::adjustMinimumTimerInterval, defaultMinTimerInterval and setDefaultMinTimerInterval. Those mechanisms were somewhat tricky to get right, have been quite well tested (see the *-min-interval-* tests in fast/dom/) and it would have been much easier, I think, to just call them in response to page visibility events. I think that when I originally added that code, the page visibility stuff might have only been triggered by code external to WebCore, but I'm not sure. One thing that is certain is that the layout tests rely on being able to change the timer interval, which is why they're exposed to DRT. I hope that a cleanup and unification of these code paths will be done in the future.
Comment 51 Kiran Muppala 2012-10-09 16:30:16 PDT
(In reply to comment #50)
> (From update of attachment 167675 [details])
> As far as I can tell, this patch basically duplicates the functionality of DOMTimer::adjustMinimumTimerInterval, defaultMinTimerInterval and setDefaultMinTimerInterval. Those mechanisms were somewhat tricky to get right, have been quite well tested (see the *-min-interval-* tests in fast/dom/) and it would have been much easier, I think, to just call them in response to page visibility events. I think that when I originally added that code, the page visibility stuff might have only been triggered by code external to WebCore, but I'm not sure. One thing that is certain is that the layout tests rely on being able to change the timer interval, which is why they're exposed to DRT. I hope that a cleanup and unification of these code paths will be done in the future.

Adjusting minimum timer interval and aligning the fire times both achieve the effect of reducing the number of timers fired, but there are differences.  Minimum timer interval does not try to coalesce timers and also it is subject to ramp-up (nesting level), a reason why it is tricky.  It might be possible to unify the two code paths, particularly if adjustMinimumTimerInterval relaxes constraints on the exact fire time.
Comment 52 Maciej Stachowiak 2012-10-09 16:42:27 PDT
(In reply to comment #50)
> (From update of attachment 167675 [details])
> As far as I can tell, this patch basically duplicates the functionality of DOMTimer::adjustMinimumTimerInterval, defaultMinTimerInterval and setDefaultMinTimerInterval.

See comment #31 for why it's not a duplicate of the functionality. We know from measurement that the first two differences lead to greater power benefits.
Comment 53 Kiran Muppala 2012-10-09 16:50:28 PDT
(In reply to comment #47)
> (From update of attachment 167675 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167675&action=review
> 
> > LayoutTests/fast/dom/timer-throttling-hidden-page.html:8
> > +	var previousT = new Date().getTime();
> 
> Could you fix up the indentation/style in this file and make the variable names more in line with WebKit style (e.g. s/previousT/previousTime)? It looks like you have tab characters in here. You should only use spaces, even for tests. We don't have an official style guide for tests, but they are something that we need to maintain, so deserve a minimum level of quality.

Fixed by https://bugs.webkit.org/show_bug.cgi?id=98828.
Comment 54 Kenneth Russell 2012-10-09 20:16:15 PDT
(In reply to comment #52)
> (In reply to comment #50)
> > (From update of attachment 167675 [details] [details])
> > As far as I can tell, this patch basically duplicates the functionality of DOMTimer::adjustMinimumTimerInterval, defaultMinTimerInterval and setDefaultMinTimerInterval.
> 
> See comment #31 for why it's not a duplicate of the functionality. We know from measurement that the first two differences lead to greater power benefits.

OK. From recollection, the reason the minimum timer interval was implemented the way it was was simply to keep the properties similar to the old code. If Apple's convinced that the differences in behavior offer significant power savings and don't break the web, then I personally don't see any reason why the Chromium port couldn't switch over to the new mechanism, and the minimum timer interval code could be deleted entirely.

Note that there is at least one outstanding issue with the current DOMTimer throttling -- http://code.google.com/p/chromium/issues/detail?id=152077 -- and it would be interesting to know whether the new timer alignment mechanism would help address that in any way (for example, perhaps the page visibility code is smarter about the presence of popups).
Comment 55 Kiran Muppala 2012-10-09 21:16:45 PDT
(In reply to comment #54)
> (In reply to comment #52)
> > (In reply to comment #50)
> > > (From update of attachment 167675 [details] [details] [details])
> > > As far as I can tell, this patch basically duplicates the functionality of DOMTimer::adjustMinimumTimerInterval, defaultMinTimerInterval and setDefaultMinTimerInterval.
> > 
> > See comment #31 for why it's not a duplicate of the functionality. We know from measurement that the first two differences lead to greater power benefits.
> 
> OK. From recollection, the reason the minimum timer interval was implemented the way it was was simply to keep the properties similar to the old code. If Apple's convinced that the differences in behavior offer significant power savings and don't break the web, then I personally don't see any reason why the Chromium port couldn't switch over to the new mechanism, and the minimum timer interval code could be deleted entirely.
> 
> Note that there is at least one outstanding issue with the current DOMTimer throttling -- http://code.google.com/p/chromium/issues/detail?id=152077 -- and it would be interesting to know whether the new timer alignment mechanism would help address that in any way (for example, perhaps the page visibility code is smarter about the presence of popups).

Page visibility code is also not immune to this.  If, the page is not the foreground tab it will be throttled.  Filed (In reply to comment #54)
> (In reply to comment #52)
> > (In reply to comment #50)
> > > (From update of attachment 167675 [details] [details] [details])
> > > As far as I can tell, this patch basically duplicates the functionality of DOMTimer::adjustMinimumTimerInterval, defaultMinTimerInterval and setDefaultMinTimerInterval.
> > 
> > See comment #31 for why it's not a duplicate of the functionality. We know from measurement that the first two differences lead to greater power benefits.
> 
> OK. From recollection, the reason the minimum timer interval was implemented the way it was was simply to keep the properties similar to the old code. If Apple's convinced that the differences in behavior offer significant power savings and don't break the web, then I personally don't see any reason why the Chromium port couldn't switch over to the new mechanism, and the minimum timer interval code could be deleted entirely.
> 
> Note that there is at least one outstanding issue with the current DOMTimer throttling -- http://code.google.com/p/chromium/issues/detail?id=152077 -- and it would be interesting to know whether the new timer alignment mechanism would help address that in any way (for example, perhaps the page visibility code is smarter about the presence of popups).

Testing showed that page visibility check does not prevent robohornet from running slower if it is backgrounded, since the presence of foreground popup windows have no effect on the check.

Filed https://bugs.webkit.org/show_bug.cgi?id=98848 for further investigation.
Comment 56 Jesus Sanchez-Palencia 2012-10-10 10:32:59 PDT
So, just giving it a shot to start a wrap up here, there are Page Visibility states, Suspend/Resume API for WK2, DOMTimer::adjustMinimumTimerInterval and now DOM timer throttling and they are all somehow related and, from a high-level overview, aiming at the same goal?
Comment 57 Ojan Vafai 2012-10-10 17:02:43 PDT
(In reply to comment #56)
> So, just giving it a shot to start a wrap up here, there are Page Visibility states, Suspend/Resume API for WK2, DOMTimer::adjustMinimumTimerInterval and now DOM timer throttling and they are all somehow related and, from a high-level overview, aiming at the same goal?

DOM timer throttling and DOMTimer::adjustMinimumTimerInterval are duplicate. Eventually, it sounds like everyone will want to do DOM Timer throttling as per this patch and we kill DOMTimer::adjustMinimumTimerInterval.

Although, since no ports seem to be objecting to this change, the shorter path to the desired end result, might be to just change DOMTimer::adjustMinimumTimerInterval to instead do the behavior from this patch.
Comment 58 Maciej Stachowiak 2012-10-10 17:07:27 PDT
(In reply to comment #57)
> (In reply to comment #56)
> > So, just giving it a shot to start a wrap up here, there are Page Visibility states, Suspend/Resume API for WK2, DOMTimer::adjustMinimumTimerInterval and now DOM timer throttling and they are all somehow related and, from a high-level overview, aiming at the same goal?
> 
> DOM timer throttling and DOMTimer::adjustMinimumTimerInterval are duplicate. Eventually, it sounds like everyone will want to do DOM Timer throttling as per this patch and we kill DOMTimer::adjustMinimumTimerInterval.
> 
> Although, since no ports seem to be objecting to this change, the shorter path to the desired end result, might be to just change DOMTimer::adjustMinimumTimerInterval to instead do the behavior from this patch.

That may result in WebCore and Chromium both attempting to set the timer interval, for the Chromium port. Seems better to let Chromium opt in to the new mechanism and stop using the old mechanism at the time of its choosing, then remove the old code once unused.

Alternately, we could have an ifdef for whether the timer alignment interval is controlled automatically by WebCore based on page visibility or explicitly by the embedder, but that seems somewhat inelegant.

I do agree with the goal of getting down to one mechanism, since Chromium folks agree with the approach taken by this patch.
Comment 59 Joseph Pecoraro 2012-10-23 16:03:44 PDT
Comment on attachment 167675 [details]
Patch

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

> Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:67
> +ENABLE_HIDDEN_PAGE_DOM_TIMER_THROTTLING = $(HIDDEN_PAGE_DOM_TIMER_THROTTLING_$(REAL_PLATFORM_NAME));
> +ENABLE_HIDDEN_PAGE_DOM_TIMER_THROTTLING_macosx = HIDDEN_PAGE_DOM_TIMER_THROTTLING;

The way this is written, I don't think this feature will actually be enabled. The "ENABLE_" prefix is missing in the value, so the _macosx value is not actually picked up.

This should most likely be:

    ENABLE_HIDDEN_PAGE_DOM_TIMER_THROTTLING = $(ENABLE_HIDDEN_PAGE_DOM_TIMER_THROTTLING_$(REAL_PLATFORM_NAME));
    ENABLE_HIDDEN_PAGE_DOM_TIMER_THROTTLING_macosx = ENABLE_HIDDEN_PAGE_DOM_TIMER_THROTTLING;

I'm working on some FeatureDefines cleanup, and I'll fix this in the process.
Comment 60 Kiran Muppala 2012-10-23 16:09:08 PDT
(In reply to comment #59)
> (From update of attachment 167675 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167675&action=review
> 
> > Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:67
> > +ENABLE_HIDDEN_PAGE_DOM_TIMER_THROTTLING = $(HIDDEN_PAGE_DOM_TIMER_THROTTLING_$(REAL_PLATFORM_NAME));
> > +ENABLE_HIDDEN_PAGE_DOM_TIMER_THROTTLING_macosx = HIDDEN_PAGE_DOM_TIMER_THROTTLING;
> 
> The way this is written, I don't think this feature will actually be enabled. The "ENABLE_" prefix is missing in the value, so the _macosx value is not actually picked up.
> 
> This should most likely be:
> 
>     ENABLE_HIDDEN_PAGE_DOM_TIMER_THROTTLING = $(ENABLE_HIDDEN_PAGE_DOM_TIMER_THROTTLING_$(REAL_PLATFORM_NAME));
>     ENABLE_HIDDEN_PAGE_DOM_TIMER_THROTTLING_macosx = ENABLE_HIDDEN_PAGE_DOM_TIMER_THROTTLING;
> 
> I'm working on some FeatureDefines cleanup, and I'll fix this in the process.

You are right, this is incorrect.  But, there were no changes in JavaScriptCore for this feature, and thats why the feature is still working. It should still be corrected to avoid surprises.  Thanks for catching it and working on cleaning it up.