Bug 135082 – [iOS][WK2] Improve event throttling for Scroll Events

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Benjamin Poulain]

[iOS][WK2] Improve event throttling for Scroll Events

[Benjamin Poulain]

Created attachment 235166 [details]
Patch

[Benjamin Poulain]

Created attachment 235168 [details]
Patch

[Sam Weinig]

Comment on attachment 235168 [details]
Patch

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

> Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.mm:372
> +        if (WebPage *webPage = WebProcess::shared().webPage(pageID)) {

I don't think it safe to call WebProcess::shared().webPage() from a background queue.

[Benjamin Poulain]

Comment on attachment 235168 [details]
Patch

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

>> Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.mm:372
>> +        if (WebPage *webPage = WebProcess::shared().webPage(pageID)) {
> 
> I don't think it safe to call WebProcess::shared().webPage() from a background queue.

Yeah, that's really dumb. That code should be in the dispatch_async(). I used the PageID specifically to avoid races and then I create one 3 lines bellow...

I'll move it in the right place.

[Simon Fraser (smfr)]

Comment on attachment 235168 [details]
Patch

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

> Source/WebCore/page/ChromeClient.h:239
> +    virtual double eventThrottlingDelay() { return 0; };

Should this use std::chrono something?

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2881
> +        m_estimatedMainThreadLatency = m_estimatedMainThreadLatency * 0.80 + elapsed * 0.20;

I'm not sure the "main thread" here is interesting.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4472
> +    m_estimatedMainThreadLatency = 1 / 60.;

60.0?

> Source/WebKit2/WebProcess/WebPage/WebPage.h:514
> +    double eventThrottlingDelay() const;

std::chrono would tell me what the units are.

> Source/WebKit2/WebProcess/WebPage/WebPage.h:754
> +    void updateVisibleContentRects(const VisibleContentRectUpdateInfo&, double);

Please give the double a name.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1680
> +    if (m_isInStableState || m_estimatedMainThreadLatency <= 1. / 60)

Why not 1/60.0 like you used before?

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1683
> +    return std::min<double>(m_estimatedMainThreadLatency * 2, 1);

Explain the * 2 in a comment. What units are we in here?

[Benjamin Poulain]

(In reply to comment #5)
> (From update of attachment 235168 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=235168&action=review
> 
> > Source/WebCore/page/ChromeClient.h:239
> > +    virtual double eventThrottlingDelay() { return 0; };
> 
> Should this use std::chrono something?

That is wasting performance and accuracy...but whatever, I'll do it.

[Benjamin Poulain]

Committed r171336: <http://trac.webkit.org/changeset/171336>

[Simon Fraser (smfr)]

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 235168 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=235168&action=review
> > 
> > > Source/WebCore/page/ChromeClient.h:239
> > > +    virtual double eventThrottlingDelay() { return 0; };
> > 
> > Should this use std::chrono something?
> 
> That is wasting performance and accuracy...but whatever, I'll do it.

Isn't it just syntactic sugar?

[Benjamin Poulain]

(In reply to comment #8)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > (From update of attachment 235168 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=235168&action=review
> > > 
> > > > Source/WebCore/page/ChromeClient.h:239
> > > > +    virtual double eventThrottlingDelay() { return 0; };
> > > 
> > > Should this use std::chrono something?
> > 
> > That is wasting performance and accuracy...but whatever, I'll do it.
> 
> Isn't it just syntactic sugar?

Nope, chrono is long integers. By picking milliseconds you lose the sub-milliseconds accuracy (like the 0.6 in 16.6ms per frame). It is also a lot more work to use long type on ARMv7 while the double were just going through VFP.

[Simon Fraser (smfr)]

(In reply to comment #9)
> Nope, chrono is long integers. By picking milliseconds you lose the sub-milliseconds accuracy (like the 0.6 in 16.6ms per frame). It is also a lot more work to use long type on ARMv7 while the double were just going through VFP.

That sucks. Feel free to go back to doubles, but name the functions (or add comments) saying what the units are.

[Benjamin Poulain]

(In reply to comment #10)
> (In reply to comment #9)
> > Nope, chrono is long integers. By picking milliseconds you lose the sub-milliseconds accuracy (like the 0.6 in 16.6ms per frame). It is also a lot more work to use long type on ARMv7 while the double were just going through VFP.
> 
> That sucks. Feel free to go back to doubles, but name the functions (or add comments) saying what the units are.

As far as I can remember, we have always used double for seconds (take Timer.h for example).

I don't think it is worth changing everything back. We don't need much perf or accuracy here.