[iOS][WK2] Improve event throttling for Scroll Events
Created attachment 235166 [details] Patch
Created attachment 235168 [details] Patch
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.
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.
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?
(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.
Committed r171336: <http://trac.webkit.org/changeset/171336>
(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?
(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.
(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.
(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.