<rdar://problem/47570443> Responsiveness timers are too expensive for frequent events
Created attachment 360549 [details] Patch
Comment on attachment 360549 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=360549&action=review r=me > Source/WebKit/ChangeLog:22 > + The second improvement of the patch is to avoid that by not killing the timer > + when we hear back from the WebProcess. I wonder if we should just make this the default behavior of WebCore::Timer or ResponsivenessTimer. > Source/WebKit/ChangeLog:30 > + In the case of wheel events, this is find since we saved a bunch of CPU already. fine
Please make sure to verify manually that you still see the SPOD cursor in response to clicks and wheel events and typing if the web process is hung.
Thanks for the review. (In reply to Geoffrey Garen from comment #2) > > Source/WebKit/ChangeLog:22 > > + The second improvement of the patch is to avoid that by not killing the timer > > + when we hear back from the WebProcess. > > I wonder if we should just make this the default behavior of WebCore::Timer > or ResponsivenessTimer. That was my original idea. The problem is every timer except fo wheel event was causing an idle wake. Idle wake can really ruin your day (e.g. rdar://problem/46298048). (In reply to Geoffrey Garen from comment #3) > Please make sure to verify manually that you still see the SPOD cursor in > response to clicks and wheel events and typing if the web process is hung. Will do.
Created attachment 360676 [details] Patch
Tested by: -Disable "platformIsBeingDebugged" -Use lldb to stop the WebProcess -Click on the page. -Type in a text field -Scroll with wheel events
Comment on attachment 360676 [details] Patch Clearing flags on attachment: 360676 Committed r240759: <https://trac.webkit.org/changeset/240759>
All reviewed patches have been landed. Closing bug.
<rdar://problem/47692525>
Comment on attachment 360676 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=360676&action=review > Source/WebKit/ChangeLog:26 > + Using WebCore::Timer saves some instructions but we were still hitting > + the kernel at 120hz to set up then kill each timer. > + The second improvement of the patch is to avoid that by not killing the timer > + when we hear back from the WebProcess. > + > + Instead of killing the timer, we let it run and ignore the result. > + When the next event comes, we reschedule the existing timer. > + This brings down the timers to 60Hz, the same rate as the events. Could this have used DeferrableOneShotTimer?
(In reply to Simon Fraser (smfr) from comment #10) > Comment on attachment 360676 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=360676&action=review > > > Source/WebKit/ChangeLog:26 > > + Using WebCore::Timer saves some instructions but we were still hitting > > + the kernel at 120hz to set up then kill each timer. > > + The second improvement of the patch is to avoid that by not killing the timer > > + when we hear back from the WebProcess. > > + > > + Instead of killing the timer, we let it run and ignore the result. > > + When the next event comes, we reschedule the existing timer. > > + This brings down the timers to 60Hz, the same rate as the events. > > Could this have used DeferrableOneShotTimer? It's a different use case. Just using DeferrableOneShotTimer does not help because we call stop() before each start()/restart(). We could combine DeferrableOneShotTimer with the lazy stop to lower the timer rescheduling to 1 timer / 3 seconds. I like the idea, I add that to my todo list :)
Reopening to attach new patch.
Created attachment 361354 [details] Patch
Attachment 361354 [details] did not pass style-queue: ERROR: Source/WebCore/platform/Timer.h:153: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 1 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 361354 [details] Patch Clearing flags on attachment: 361354 Committed r241113: <https://trac.webkit.org/changeset/241113>
This was rolled out in https://trac.webkit.org/changeset/241113/webkit
The rollout commit says "Some timer uses are done off the main thread" but WebCore::Timer also can't be used in the WK2 UI process because of co-existence with the Web-Thread-enabled WK1.
Reopening, the optimization was reverted.
Created attachment 361766 [details] Patch
Comment on attachment 361766 [details] Patch Attachment 361766 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/11118407 New failing tests: http/tests/inspector/network/resource-initiatorNode.html
Created attachment 361773 [details] Archive of layout-test-results from ews115 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-highsierra Platform: Mac OS X 10.13.6
Created attachment 361777 [details] Patch
Comment on attachment 361777 [details] Patch r=me
Comment on attachment 361777 [details] Patch (In reply to Geoffrey Garen from comment #24) > Comment on attachment 361777 [details] > Patch > > r=me Thanks for the review!
Comment on attachment 361777 [details] Patch Clearing flags on attachment: 361777 Committed r241351: <https://trac.webkit.org/changeset/241351>