Bug 188005 – [GTK][WPE] Improve the way request displayRefresh notifications

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Miguel Gomez]

Currently, when we want to receive displayRefresh notifications, we directly schedule updates from ThreadedCompositor::requestDisplayRefreshMonitorUpdate and ThreadedCompositor::handleDisplayRefreshMonitorUpdate. But these updates are not properly coordinated with updates that may come from layer flushes, and they cause delays and even dropped frames in some animations that use rAF.

What should happen instead is that when we want to force repaints, we should perform a layer flush and then a painting that reflects the changes caused during that flush.

This way there are only 3 possibilities to cause repaints in the ThreadedCompositor:
- layer flush updates
- animations handled in the compositor thread
- arrival of new video frames

[Miguel Gomez]

Created attachment 345762 [details]
Patch

[EWS Watchlist]

Attachment 345762 [details] did not pass style-queue:


ERROR: Source/WebKit/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp:59:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Zan Dobersek]

Comment on attachment 345762 [details]
Patch

Looks OK, but let me apply & test it.

[WebKit Commit Bot]

Comment on attachment 345762 [details]
Patch

Clearing flags on attachment: 345762

Committed r234259: <https://trac.webkit.org/changeset/234259>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Carlos Garcia Campos]

I've noticed that lately the matrix web applications uses a lot more CPU in the web process, and I've checked that WTF::memoryFootprint() is called often. That comes from WTF::MemoryPressureHandler::currentMemoryUsagePolicy() used to decide the compositing policy, but the origin is in the threaded refresh monitor. So, I wonder if this patch could be the cause. It happens even when the application is not visible, the bt is always the same:

Thread 1 (Thread 0x7fe2db0409c0 (LWP 30970)):
#0  0x00007fe2e1ae5204 in __GI___libc_read (fd=67, buf=0x55a38c0716d0, nbytes=1024) at ../sysdeps/unix/sysv/linux/read.c:27
#1  0x00007fe2e1a77838 in _IO_new_file_underflow (fp=0x55a38c24cd00) at fileops.c:531
#2  0x00007fe2e1a6b9ac in _IO_getdelim (lineptr=0x7ffccbe42a50, n=0x7ffccbe42a58, delimiter=10, fp=0x55a38c24cd00) at iogetdelim.c:114
#3  0x00007fe2ed801fd7 in WTF::memoryFootprint() () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.0.so.37
#4  0x00007fe2ed7f97f6 in WTF::MemoryPressureHandler::currentMemoryUsagePolicy() () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.0.so.37
#5  0x00007fe2ed1e1cdd in WebCore::RenderLayerCompositor::updateCompositingPolicy() () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.0.so.37
#6  0x00007fe2ed1e1eec in WebCore::RenderLayerCompositor::cacheAcceleratedCompositingFlags() () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.0.so.37
#7  0x00007fe2eca6301e in WebCore::Document::resolveStyle(WebCore::Document::ResolveStyleType) () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.0.so.37
#8  0x00007fe2eca6387a in WebCore::Document::updateStyleIfNeeded() () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.0.so.37
#9  0x00007fe2ec84b372 in WebCore::DocumentTimeline::updateAnimations() () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.0.so.37
#10 0x00007fe2ec84b585 in WebCore::DocumentAnimationScheduler::displayRefreshFired() () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.0.so.37
#11 0x00007fe2ecfc77be in WebCore::DisplayRefreshMonitor::displayDidRefresh() () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.0.so.37
#12 0x00007fe2ec118e38 in WebKit::ThreadedDisplayRefreshMonitor::displayRefreshCallback() () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.0.so.37
#13 0x00007fe2e9c5fa03 in WTF::RunLoop::TimerBase::TimerBase(WTF::RunLoop&)::{lambda(void*)#1}::_FUN(void*) () from /home/cgarcia/gnome/lib/libjavascriptcoregtk-4.0.so.18
#14 0x00007fe2e3c2a495 in g_main_dispatch (context=0x55a38b668460) at gmain.c:3177
#15 g_main_context_dispatch (context=context@entry=0x55a38b668460) at gmain.c:3830
#16 0x00007fe2e3c2a838 in g_main_context_iterate (context=0x55a38b668460, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at gmain.c:3903
#17 0x00007fe2e3c2ab42 in g_main_loop_run (loop=0x55a38b6eb6e0) at gmain.c:4099
#18 0x00007fe2e9c5fe20 in WTF::RunLoop::run() () from /home/cgarcia/gnome/lib/libjavascriptcoregtk-4.0.so.18
#19 0x00007fe2ec269630 in int WebKit::ChildProcessMain<WebKit::WebProcess, WebKit::WebProcessMain>(int, char**) () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.0.so.37
#20 0x00007fe2e1a1eb17 in __libc_start_main (main=0x55a389a00c30 <main>, argc=3, argv=0x7ffccbe434d8, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, 
    stack_end=0x7ffccbe434c8) at ../csu/libc-start.c:310
#21 0x000055a389a00cba in _start ()

I had to disable AC for matrix. I wonder why matrix uses AC all the time too, with the ondemand mode, it used to be disabled most of the time. Now it seems to be always enabled and the threaded refresh monitor is being fired even when there isn't any animation and the web view is not even visible or active.

[Carlos Garcia Campos]

I've reverted this and my laptops fans are quiet again, so this seems to be the cause of the excessive cpu usage. Since we have just branched, I'll revert it only in the stable branch, once the fix lands in trunk I'll backport both patches.

[Michael Catanzaro]

I suggest we revert it in trunk too. Let's keep trunk in good state. We don't want this to come back to surprise us in six months. And if nothing else, it's important for Ephy Tech Preview.

No doubt Miguel will be able to look at this when he returns.

[Michael Catanzaro]

We agreed to rollout and wait for Miguel.

[Michael Catanzaro]

Reverted r234259 for reason:

Caused excessive CPU usage

Committed r234981: <https://trac.webkit.org/changeset/234981>

[Zan Dobersek]

(In reply to Carlos Garcia Campos from comment #6)
> I've noticed that lately the matrix web applications uses a lot more CPU in
> the web process, and I've checked that WTF::memoryFootprint() is called
> often. That comes from
> WTF::MemoryPressureHandler::currentMemoryUsagePolicy() used to decide the
> compositing policy, but the origin is in the threaded refresh monitor. So, I
> wonder if this patch could be the cause. It happens even when the
> application is not visible, the bt is always the same:
> 
> Thread 1 (Thread 0x7fe2db0409c0 (LWP 30970)):
> #0  0x00007fe2e1ae5204 in __GI___libc_read (fd=67, buf=0x55a38c0716d0,
> nbytes=1024) at ../sysdeps/unix/sysv/linux/read.c:27
> #1  0x00007fe2e1a77838 in _IO_new_file_underflow (fp=0x55a38c24cd00) at
> fileops.c:531
> #2  0x00007fe2e1a6b9ac in _IO_getdelim (lineptr=0x7ffccbe42a50,
> n=0x7ffccbe42a58, delimiter=10, fp=0x55a38c24cd00) at iogetdelim.c:114
> #3  0x00007fe2ed801fd7 in WTF::memoryFootprint() () from
> /home/cgarcia/gnome/lib/libwebkit2gtk-4.0.so.37
> #4  0x00007fe2ed7f97f6 in
> WTF::MemoryPressureHandler::currentMemoryUsagePolicy() () from
> /home/cgarcia/gnome/lib/libwebkit2gtk-4.0.so.37
> #5  0x00007fe2ed1e1cdd in
> WebCore::RenderLayerCompositor::updateCompositingPolicy() () from
> /home/cgarcia/gnome/lib/libwebkit2gtk-4.0.so.37
> #6  0x00007fe2ed1e1eec in
> WebCore::RenderLayerCompositor::cacheAcceleratedCompositingFlags() () from
> /home/cgarcia/gnome/lib/libwebkit2gtk-4.0.so.37
> #7  0x00007fe2eca6301e in
> WebCore::Document::resolveStyle(WebCore::Document::ResolveStyleType) () from
> /home/cgarcia/gnome/lib/libwebkit2gtk-4.0.so.37
> #8  0x00007fe2eca6387a in WebCore::Document::updateStyleIfNeeded() () from
> /home/cgarcia/gnome/lib/libwebkit2gtk-4.0.so.37
> #9  0x00007fe2ec84b372 in WebCore::DocumentTimeline::updateAnimations() ()
> from /home/cgarcia/gnome/lib/libwebkit2gtk-4.0.so.37
> #10 0x00007fe2ec84b585 in
> WebCore::DocumentAnimationScheduler::displayRefreshFired() () from
> /home/cgarcia/gnome/lib/libwebkit2gtk-4.0.so.37
> #11 0x00007fe2ecfc77be in
> WebCore::DisplayRefreshMonitor::displayDidRefresh() () from
> /home/cgarcia/gnome/lib/libwebkit2gtk-4.0.so.37
> #12 0x00007fe2ec118e38 in
> WebKit::ThreadedDisplayRefreshMonitor::displayRefreshCallback() () from
> /home/cgarcia/gnome/lib/libwebkit2gtk-4.0.so.37
> #13 0x00007fe2e9c5fa03 in
> WTF::RunLoop::TimerBase::TimerBase(WTF::RunLoop&)::{lambda(void*)#1}::
> _FUN(void*) () from /home/cgarcia/gnome/lib/libjavascriptcoregtk-4.0.so.18
> #14 0x00007fe2e3c2a495 in g_main_dispatch (context=0x55a38b668460) at
> gmain.c:3177
> #15 g_main_context_dispatch (context=context@entry=0x55a38b668460) at
> gmain.c:3830
> #16 0x00007fe2e3c2a838 in g_main_context_iterate (context=0x55a38b668460,
> block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at
> gmain.c:3903
> #17 0x00007fe2e3c2ab42 in g_main_loop_run (loop=0x55a38b6eb6e0) at
> gmain.c:4099
> #18 0x00007fe2e9c5fe20 in WTF::RunLoop::run() () from
> /home/cgarcia/gnome/lib/libjavascriptcoregtk-4.0.so.18
> #19 0x00007fe2ec269630 in int WebKit::ChildProcessMain<WebKit::WebProcess,
> WebKit::WebProcessMain>(int, char**) () from
> /home/cgarcia/gnome/lib/libwebkit2gtk-4.0.so.37
> #20 0x00007fe2e1a1eb17 in __libc_start_main (main=0x55a389a00c30 <main>,
> argc=3, argv=0x7ffccbe434d8, init=<optimized out>, fini=<optimized out>,
> rtld_fini=<optimized out>, 
>     stack_end=0x7ffccbe434c8) at ../csu/libc-start.c:310
> #21 0x000055a389a00cba in _start ()
> 
> I had to disable AC for matrix. I wonder why matrix uses AC all the time
> too, with the ondemand mode, it used to be disabled most of the time. Now it
> seems to be always enabled and the threaded refresh monitor is being fired
> even when there isn't any animation and the web view is not even visible or
> active.

This problem is more likely due to r234330, which introduced repeated memory policy checks. But more indirectly, high CPU usage is due to the wasteful way that the memory footprint value is gathered on Linux.
https://trac.webkit.org/changeset/234330/webkit

[Zan Dobersek]

(In reply to Zan Dobersek from comment #11)
> This problem is more likely due to r234330, which introduced repeated memory
> policy checks. But more indirectly, high CPU usage is due to the wasteful
> way that the memory footprint value is gathered on Linux.
> https://trac.webkit.org/changeset/234330/webkit

Confirmed, CPU usage goes bonkers with or without this patch on the following site:
https://inexorabletash.github.io/polyfill/demos/raf.html

[Michael Catanzaro]

Should we revert this rollout, then?

[Michael Catanzaro]

(In reply to Zan Dobersek from comment #12)
> Confirmed, CPU usage goes bonkers with or without this patch on the
> following site:
> https://inexorabletash.github.io/polyfill/demos/raf.html

Warning: don't open that page unless you're prepared to cut power to your machine :(

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #14)
> Warning: don't open that page unless you're prepared to cut power to your
> machine :(

Ah, it's fine, I just forgot I was compiling stuff at the same time.

[Carlos Garcia Campos]

without the patch the refresh monitor is not firing all the time in matrix app.

[Zan Dobersek]

(In reply to Carlos Garcia Campos from comment #16)
> without the patch the refresh monitor is not firing all the time in matrix
> app.

That's not necessarily wrong. We need extra flushes for correct display of the latest scene state in combination with requestAnimationFrame() requests.

Layer flushes on their own can still be expensive because of potential painting that might be done during the process. That can't be improved ATM.

But the pain point here is the way we gather memory footprint information. That's too expensive to be done on every frame.

[Carlos Garcia Campos]

(In reply to Zan Dobersek from comment #17)
> (In reply to Carlos Garcia Campos from comment #16)
> > without the patch the refresh monitor is not firing all the time in matrix
> > app.
> 
> That's not necessarily wrong. We need extra flushes for correct display of
> the latest scene state in combination with requestAnimationFrame() requests.

But this is happening all the time, even when nothing is happening in the app, and even when the matrix app is in the background. I would expect that a) AC mode should be disabled in those cases, b) even in AC mode, no layer flushes happen, painting should be suspended. For some reason it seems that WebKit thinks there's an active animation, bu there's nothing.

> Layer flushes on their own can still be expensive because of potential
> painting that might be done during the process. That can't be improved ATM.
> 
> But the pain point here is the way we gather memory footprint information.
> That's too expensive to be done on every frame.

But that's a separated issue, memory footprint parsing wasn't supposed to be called so often and it's an expensive operation. We could try to improve that too, but I think it's not the main cause of this issue.

[Zan Dobersek]

(In reply to Carlos Garcia Campos from comment #18)
> (In reply to Zan Dobersek from comment #17)
> > (In reply to Carlos Garcia Campos from comment #16)
> > > without the patch the refresh monitor is not firing all the time in matrix
> > > app.
> > 
> > That's not necessarily wrong. We need extra flushes for correct display of
> > the latest scene state in combination with requestAnimationFrame() requests.
> 
> But this is happening all the time, even when nothing is happening in the
> app, and even when the matrix app is in the background. I would expect that
> a) AC mode should be disabled in those cases, b) even in AC mode, no layer
> flushes happen, painting should be suspended. For some reason it seems that
> WebKit thinks there's an active animation, bu there's nothing.
> 

If the Matrix app is in the background but animations are still going, then there's a problem with composition suspension.

But even when in the foreground, if the app request an animation frame but changes nothing in the content, the frame notification has to be delivered. This patch enforced layer flushes in these cases because only this way we can correctly handle any pending changes in the content.

> > Layer flushes on their own can still be expensive because of potential
> > painting that might be done during the process. That can't be improved ATM.
> > 
> > But the pain point here is the way we gather memory footprint information.
> > That's too expensive to be done on every frame.
> 
> But that's a separated issue, memory footprint parsing wasn't supposed to be
> called so often and it's an expensive operation. We could try to improve
> that too, but I think it's not the main cause of this issue.

Why is it not the main cause of this issue? It's what was causing the high CPU usage on the Matrix app, which is still a problem on any continuous animation example. Revert of this patch only fixed the Matrix app.

[Michael Catanzaro]

The rollout has been rolled out -> RESOLVED FIXED.