Bug 169236 – Port DOMTimer from std::chrono::milliseconds to WTF::Seconds type

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Chris Dumez]

Port DOMTimer from std::chrono::milliseconds to WTF::Seconds type.

[Chris Dumez]

Created attachment 303581 [details]
Patch

[Chris Dumez]

Created attachment 303583 [details]
Patch

[Simon Fraser (smfr)]

See bug 165079

[Chris Dumez]

(In reply to comment #3)
> See bug 165079

Hmm, I wasn't aware of this change. Sad that it has not landed since last year.
How do you want to proceed? My patch should be building now and has more limited scope apparently.

[Chris Dumez]

Created attachment 303586 [details]
Patch

[Chris Dumez]

Should take care of the debug build.

[Simon Fraser (smfr)]

Comment on attachment 303583 [details]
Patch

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

> Source/WebCore/page/DOMTimer.cpp:56
> +static const Seconds maxIntervalForUserGestureForwarding { 1 }; // One second matches Gecko.
> +static const Seconds minIntervalForNonUserObservableChangeTimers { 1 }; // Empirically determined to maximize battery life.

1_s

> Source/WebCore/page/DOMTimer.cpp:417
> +    Seconds interval = std::max(Seconds::fromMilliseconds(1), m_originalInterval);

1_ms

> Source/WebCore/page/DOMTimer.cpp:441
> +    auto randomizedOffsetMS = static_cast<long long>(alignmentIntervalMS * randomizedProportion);
> +    auto adjustedFireTimeMS = static_cast<long long>(fireTime.milliseconds()) - randomizedOffsetMS;

Are you keeping these as long long explicitly for discretization? I used MonotonicTime for the adjustedFireTime.

> Source/WebCore/page/DOMTimer.h:48
> +    static Seconds defaultMinimumInterval() { return Seconds::fromMilliseconds(4); }

4_ms

> Source/WebCore/page/DOMTimer.h:50
> +    static Seconds hiddenPageAlignmentInterval() { return Seconds { 1 }; }

1_s

> Source/WebCore/page/Page.cpp:1364
> +            m_domTimerAlignmentInterval = Seconds { monotonicallyIncreasingTime() - m_timerThrottlingStateLastChangedTime };

Use MonotonicTime, not monotonicallyIncreasingTime()

> Source/WebCore/page/Page.h:718
> +    Seconds m_domTimerAlignmentIntervalIncreaseLimit { 0 };

No need to initialize a Seconds.

> Source/WebCore/platform/Timer.cpp:385
> +        if (auto newAlignedTime = alignedFireTime(Seconds { newTime }))

Are we doing Seconds { newTime } now, instead of just Seconds(newTime)?

> Source/WebCore/testing/Internals.cpp:1039
> +    return !!timer->alignedFireTime(Seconds { 0 });

0_s

[Chris Dumez]

Comment on attachment 303583 [details]
Patch

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

>> Source/WebCore/page/DOMTimer.cpp:56
>> +static const Seconds minIntervalForNonUserObservableChangeTimers { 1 }; // Empirically determined to maximize battery life.
> 
> 1_s

Oh, I did not know about these suffixes. I'll use them everywhere.

>> Source/WebCore/page/DOMTimer.cpp:441
>> +    auto adjustedFireTimeMS = static_cast<long long>(fireTime.milliseconds()) - randomizedOffsetMS;
> 
> Are you keeping these as long long explicitly for discretization? I used MonotonicTime for the adjustedFireTime.

I was trying to keep the code as close to the existing one as possible because it is a bit complicated and I worry about introducing bugs. For example, the code used to deal with milliseconds and I would need to look into it more to make sure it does the same thing with seconds.

>> Source/WebCore/page/Page.cpp:1364
>> +            m_domTimerAlignmentInterval = Seconds { monotonicallyIncreasingTime() - m_timerThrottlingStateLastChangedTime };
> 
> Use MonotonicTime, not monotonicallyIncreasingTime()

I did not know about MonotonicTime, I'll use it here.

>> Source/WebCore/page/Page.h:718
>> +    Seconds m_domTimerAlignmentIntervalIncreaseLimit { 0 };
> 
> No need to initialize a Seconds.

Ok.

>> Source/WebCore/platform/Timer.cpp:385
>> +        if (auto newAlignedTime = alignedFireTime(Seconds { newTime }))
> 
> Are we doing Seconds { newTime } now, instead of just Seconds(newTime)?

Darin coding style :)

[Chris Dumez]

Created attachment 303606 [details]
Patch

[Chris Dumez]

Created attachment 303655 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 303655 [details]
Patch

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

> Source/WebCore/page/DOMTimer.cpp:404
> +        LOG(DOMTimers, "%p - Updating DOMTimer's repeat interval from %f ms to %f ms due to throttling.", this, previousInterval.milliseconds(), m_currentTimerInterval.milliseconds());

Maybe %.2f in these logs for readability.

> Source/WebCore/page/DOMTimer.cpp:442
> +    Seconds randomizedOffset = alignmentInterval * randomizedProportion;
> +    Seconds adjustedFireTime = fireTime - randomizedOffset;
>      return adjustedFireTime - (adjustedFireTime % alignmentInterval) + alignmentInterval + randomizedOffset;

Did you do some runtime logging/debugging to make sure that this code still results in good alignment?

[Chris Dumez]

(In reply to comment #11)
> Comment on attachment 303655 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=303655&action=review
> 
> > Source/WebCore/page/DOMTimer.cpp:404
> > +        LOG(DOMTimers, "%p - Updating DOMTimer's repeat interval from %f ms to %f ms due to throttling.", this, previousInterval.milliseconds(), m_currentTimerInterval.milliseconds());
> 
> Maybe %.2f in these logs for readability.

Ok.

> 
> > Source/WebCore/page/DOMTimer.cpp:442
> > +    Seconds randomizedOffset = alignmentInterval * randomizedProportion;
> > +    Seconds adjustedFireTime = fireTime - randomizedOffset;
> >      return adjustedFireTime - (adjustedFireTime % alignmentInterval) + alignmentInterval + randomizedOffset;
> 
> Did you do some runtime logging/debugging to make sure that this code still
> results in good alignment?

Yes.

[Chris Dumez]

Created attachment 303658 [details]
Patch

[Chris Dumez]

Comment on attachment 303658 [details]
Patch

Clearing flags on attachment: 303658

Committed r213519: <http://trac.webkit.org/changeset/213519>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.