Bug 98474 – Throttle DOM timers on hidden pages.

• Component: DOM
• Status: RESOLVED FIXED

[Kiran Muppala]

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.

[Kiran Muppala]

<rdar://problem/9692227>

[Kiran Muppala]

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.

[Kiran Muppala]

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.

[Kiran Muppala]

Created attachment 167261 [details]
Patch

[Build Bot]

Comment on attachment 167261 [details]
Patch

Attachment 167261 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14176408

[Kiran Muppala]

Created attachment 167289 [details]
Patch

[Jesus Sanchez-Palencia]

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.

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.

[Kiran Muppala]

(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.

[Kiran Muppala]

(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.

[Alexey Proskuryakov]

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.

[Simon Fraser (smfr)]

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

[Kiran Muppala]

(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.

[Kiran Muppala]

Created attachment 167389 [details]
Patch

[Simon Fraser (smfr)]

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?

[Kiran Muppala]

(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.

[Kiran Muppala]

(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.

[Kiran Muppala]

(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.

[Simon Fraser (smfr)]

(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

[Allan Sandfeld Jensen]

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.

[Kiran Muppala]

(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.

[Adam Barth]

I know Chromium does this as well.  I wonder if it would make sense to unify the mechanisms.

[Maciej Stachowiak]

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.

[Maciej Stachowiak]

(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.

[Maciej Stachowiak]

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.

[Kiran Muppala]

(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.

[Maciej Stachowiak]

(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.

[Maciej Stachowiak]

(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.

[Maciej Stachowiak]

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

[Maciej Stachowiak]

Comment on attachment 167389 [details]
Patch

Did not mean to mark "review+" just yet since I haven't closely reviewed all the code.

[Maciej Stachowiak]

(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.

[Kiran Muppala]

Created attachment 167464 [details]
Patch

[Kiran Muppala]

(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.

[Kiran Muppala]

Created attachment 167465 [details]
Patch

[Kiran Muppala]

(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.

[Kiran Muppala]

(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.

[Maciej Stachowiak]

Comment on attachment 167465 [details]
Patch

Looks good to me and it looks like the prior comments have been addressed.

[WebKit Review Bot]

Comment on attachment 167465 [details]
Patch

Rejecting attachment 167465 [details] from commit-queue.

[e-mail] 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.

[WebKit Review Bot]

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

[Kiran Muppala]

Created attachment 167675 [details]
Patch

[Kiran Muppala]

(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.

[Maciej Stachowiak]

Comment on attachment 167675 [details]
Patch

r=me

[WebKit Review Bot]

Comment on attachment 167675 [details]
Patch

Clearing flags on attachment: 167675

Committed r130720: <http://trac.webkit.org/changeset/130720>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kent Tamura]

Would you update http://trac.webkit.org/wiki/FeatureFlags please?

[Kiran Muppala]

(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.

[Ojan Vafai]

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.

[Ojan Vafai]

> - 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.

[James Robinson]

(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.

[Kenneth Russell]

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.

[Kiran Muppala]

(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.

[Maciej Stachowiak]

(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.

[Kiran Muppala]

(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.

[Kenneth Russell]

(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).

[Kiran Muppala]

(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.

[Jesus Sanchez-Palencia]

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?

[Ojan Vafai]

(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.

[Maciej Stachowiak]

(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.

[Joseph Pecoraro]

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.

[Kiran Muppala]

(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.