Bug 112345

Summary: [CoordGfx] requestAnimationFrame performance issues
Product: WebKit Reporter: Milian Wolff <milian.wolff>
Component: WebKit QtAssignee: Noam Rosenthal <noam>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, cmarcelo, commit-queue, helder.correia, jturcotte, luiz, noam, rafael.lobo, zeno
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 116055    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Milian Wolff 2013-03-14 07:19:39 PDT
See also: https://lists.webkit.org/pipermail/webkit-qt/2013-March/003546.html

Short version is: window.requestAnimationFrame has performance issues on QtWebKit and performs worse than a simple window.setTimeout approach.

Code to reproduce the issue:

``` test.qml ```
import QtQuick 2.0
import QtWebKit 3.0
import QtWebKit.experimental 1.0

WebView {
  id: webView
  height: 500
  width: 500
  url: "test.html"
  experimental {
    preferences.developerExtrasEnabled: true
  }
}
``````

``` test.html ```
<DOCTYPE html>
<html>
  <head>
    <title>rAF test</title>
    <script type="text/javascript">
      function animateRAF() {
        window.requestAnimationFrame(animateRAF);
      }
      function animateTimer() {
        window.setTimeout(17, animateTimer)
      }
      // abysmal performance
      window.onload = animateRAF;
      // nice performance
      // window.onload = animateTimer;
    </script>
  <body>
    <h1>rAF test</h1>
  </body>
</html>
``````

The animateRAF version above makes my system noticeably sluggish and I see a high CPU load:
  PID USER      PR  NI  VIRT  RES  SHR S  %CPU %MEM     TIME+ COMMAND                                                                        
25016 milian    20   0 2578m  55m  36m S  54.9  0.7   0:10.93 qmlscene                                                                       
  312 root      20   0  206m  92m  65m S  48.2  1.2   8:41.84 X                                                                              
25024 milian    30  10 1827m  48m  38m S  12.0  0.6   0:01.89 QtWebProcess

Using animateTimer instead I do not see any of the above processed in the top 20 of top at all.

I am using a git checkout of WebKit master with bd8ce398ed4dd2ce92f8db61d7e97b43a534297a, i.e. https://bugs.webkit.org/show_bug.cgi?id=112095 applied. That helped a bit but rAF is still unusable for me.

perf top -G, while running the rAF version of the above, shows this:

-  46.09%  [kernel]                             [k] ioread32                                                                                ◆
   - ioread32                                                                                                                               ▒
      - 53.13% 0xffffffffa05e80f6                                                                                                           ▒
         - 68.79% nouveau_gem_ioctl_pushbuf                                                                                                 ▒
              0xffffffffa02ee3f3                                                                                                            ▒
              do_vfs_ioctl                                                                                                                  ▒
              sys_ioctl                                                                                                                     ▒
              system_call_fastpath                                                                                                          ▒
              __GI___ioctl                                                                                                                  ▒
         - 31.21% nv84_fence_sync                                                                                                           ▒
              nouveau_fence_sync                                                                                                            ▒
              validate_sync.isra.3                                                                                                          ▒
              validate_list                                                                                                                 ▒
              nouveau_gem_ioctl_pushbuf                                                                                                     ▒
              drm_ioctl                                                                                                                     ▒
              do_vfs_ioctl                                                                                                                  ▒
              sys_ioctl                                                                                                                     ▒
              system_call_fastpath                                                                                                          ▒
              __GI___ioctl                                                                                                                  ▒
        34.81% _nouveau_gpuobj_rd32                                                                                                         ▒
           nv84_fence_read                                                                                                                  ▒
           nouveau_fence_done                                                                                                               ▒
        9.60% nouveau_dma_wait                                                                                                              ▒
        1.17% nv50_crtc_cursor_set                                                                                                          ▒
           drm_mode_cursor_ioctl                                                                                                            ▒
           drm_ioctl                                                                                                                        ▒
           do_vfs_ioctl                                                                                                                     ▒
           sys_ioctl                                                                                                                        ▒
           system_call_fastpath                                                                                                             ▒
           __GI___ioctl                                                                                                                     ▒
        0.88% nv50_vm_flush

I.e. the painting seems to be an issue. So apparently rAF triggers excessive repaints. Is this maybe also a driver problem of noveau?
Comment 1 Noam Rosenthal 2013-05-07 14:52:00 PDT
Created attachment 200980 [details]
Patch
Comment 2 Noam Rosenthal 2013-05-09 14:10:55 PDT
Created attachment 201276 [details]
Patch
Comment 3 Rafael Brandao 2013-05-09 14:21:25 PDT
Comment on attachment 201276 [details]
Patch

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

Very nice patch!

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:436
> +    m_lastAnimationServiceTime = WTF::monotonicallyIncreasingTime();

I think you could pass m_lastAnimationServiceTime as parameter to serviceScriptedAnimations, since in practice we are doing the same work twice.
Comment 4 Noam Rosenthal 2013-05-09 14:31:02 PDT
(In reply to comment #3)
> (From update of attachment 201276 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=201276&action=review
> 
> Very nice patch!
> 
> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:436
> > +    m_lastAnimationServiceTime = WTF::monotonicallyIncreasingTime();
> 
> I think you could pass m_lastAnimationServiceTime as parameter to serviceScriptedAnimations, since in practice we are doing the same work twice.

Right, of course :)
Comment 5 Noam Rosenthal 2013-05-09 15:17:34 PDT
Created attachment 201288 [details]
Patch
Comment 6 Jocelyn Turcotte 2013-05-13 04:53:28 PDT
Comment on attachment 201288 [details]
Patch

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

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:664
> +    m_layerFlushTimer.startOneShot(std::max<double>(0., WTF::monotonicallyIncreasingTime() + MinimalTimeoutForAnimations - m_lastAnimationServiceTime));

If we're 5ms into the frame, I think the timer should be scheduled for 11ms to reach the target, but that's not what we'll get here.
Example:
WTF::monotonicallyIncreasingTime() = 37, MinimalTimeoutForAnimations = 16, m_lastAnimationServiceTime = 32.
WTF::monotonicallyIncreasingTime() + MinimalTimeoutForAnimations - m_lastAnimationServiceTime = 37 + 16 - 32 = 21
So maybe it should be: MinimalTimeoutForAnimations - (WTF::monotonicallyIncreasingTime() - m_lastAnimationServiceTime) = 16 - (37 - 32) = 11

I'm also not sure if this can be lower than zero assuming that (WTF::monotonicallyIncreasingTime() >= m_lastAnimationServiceTime) always holds.
Comment 7 Noam Rosenthal 2013-05-13 05:13:27 PDT
Comment on attachment 201288 [details]
Patch

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

>> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:664
>> +    m_layerFlushTimer.startOneShot(std::max<double>(0., WTF::monotonicallyIncreasingTime() + MinimalTimeoutForAnimations - m_lastAnimationServiceTime));
> 
> If we're 5ms into the frame, I think the timer should be scheduled for 11ms to reach the target, but that's not what we'll get here.
> Example:
> WTF::monotonicallyIncreasingTime() = 37, MinimalTimeoutForAnimations = 16, m_lastAnimationServiceTime = 32.
> WTF::monotonicallyIncreasingTime() + MinimalTimeoutForAnimations - m_lastAnimationServiceTime = 37 + 16 - 32 = 21
> So maybe it should be: MinimalTimeoutForAnimations - (WTF::monotonicallyIncreasingTime() - m_lastAnimationServiceTime) = 16 - (37 - 32) = 11
> 
> I'm also not sure if this can be lower than zero assuming that (WTF::monotonicallyIncreasingTime() >= m_lastAnimationServiceTime) always holds.

Oops, it should be  + MinimalTimeoutForAnimations + m_lastAnimationServiceTime - WTF::monotonicallyIncreasingTime()
Comment 8 Noam Rosenthal 2013-05-13 07:05:50 PDT
Created attachment 201559 [details]
Patch
Comment 9 Jocelyn Turcotte 2013-05-13 07:36:29 PDT
Comment on attachment 201559 [details]
Patch

LGTM
Comment 10 Noam Rosenthal 2013-05-13 07:43:17 PDT
(In reply to comment #9)
> (From update of attachment 201559 [details])
> LGTM
Thanks! Based on the webkit summit we're allowed to r+ patches that touch only port-specific directories (including CoordinatedGraphics) without WK2 owner review. Guess we're still waiting for the email about it from Sam...
Comment 11 Jocelyn Turcotte 2013-05-13 07:59:17 PDT
Comment on attachment 201559 [details]
Patch

(In reply to comment #10)
> > LGTM
> Thanks! Based on the webkit summit we're allowed to r+ patches that touch only port-specific directories (including CoordinatedGraphics) without WK2 owner review. Guess we're still waiting for the email about it from Sam...

Ok, that's good to know :)
Comment 12 WebKit Commit Bot 2013-05-13 08:28:27 PDT
Comment on attachment 201559 [details]
Patch

Clearing flags on attachment: 201559

Committed r150015: <http://trac.webkit.org/changeset/150015>
Comment 13 WebKit Commit Bot 2013-05-13 08:28:30 PDT
All reviewed patches have been landed.  Closing bug.