Bug 194003 - Responsiveness timers are too expensive for frequent events
Summary: Responsiveness timers are too expensive for frequent events
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Benjamin Poulain
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-01-29 20:45 PST by Benjamin Poulain
Modified: 2019-02-13 00:41 PST (History)
6 users (show)

See Also:


Attachments
Patch (8.25 KB, patch)
2019-01-29 21:13 PST, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (8.44 KB, patch)
2019-01-30 19:28 PST, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (19.35 KB, patch)
2019-02-06 18:00 PST, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (12.44 KB, patch)
2019-02-11 21:38 PST, Benjamin Poulain
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews115 for mac-highsierra (2.08 MB, application/zip)
2019-02-11 23:27 PST, Build Bot
no flags Details
Patch (12.44 KB, patch)
2019-02-12 00:26 PST, Benjamin Poulain
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Poulain 2019-01-29 20:45:17 PST
<rdar://problem/47570443> Responsiveness timers are too expensive for frequent events
Comment 1 Benjamin Poulain 2019-01-29 21:13:35 PST
Created attachment 360549 [details]
Patch
Comment 2 Geoffrey Garen 2019-01-30 14:27:47 PST
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
Comment 3 Geoffrey Garen 2019-01-30 14:28:30 PST
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.
Comment 4 Benjamin Poulain 2019-01-30 16:44:16 PST
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.
Comment 5 Benjamin Poulain 2019-01-30 19:28:00 PST
Created attachment 360676 [details]
Patch
Comment 6 Benjamin Poulain 2019-01-30 19:37:37 PST
Tested by:
-Disable "platformIsBeingDebugged"
-Use lldb to stop the WebProcess
-Click on the page.
-Type in a text field
-Scroll with wheel events
Comment 7 WebKit Commit Bot 2019-01-30 20:53:41 PST
Comment on attachment 360676 [details]
Patch

Clearing flags on attachment: 360676

Committed r240759: <https://trac.webkit.org/changeset/240759>
Comment 8 WebKit Commit Bot 2019-01-30 20:53:42 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2019-01-30 20:54:34 PST
<rdar://problem/47692525>
Comment 10 Simon Fraser (smfr) 2019-01-30 22:01:49 PST
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?
Comment 11 Benjamin Poulain 2019-01-30 23:13:17 PST
(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 :)
Comment 12 Benjamin Poulain 2019-02-06 18:00:49 PST
Reopening to attach new patch.
Comment 13 Benjamin Poulain 2019-02-06 18:00:49 PST
Created attachment 361354 [details]
Patch
Comment 14 Build Bot 2019-02-06 18:04:01 PST
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 15 WebKit Commit Bot 2019-02-06 19:13:17 PST
Comment on attachment 361354 [details]
Patch

Clearing flags on attachment: 361354

Committed r241113: <https://trac.webkit.org/changeset/241113>
Comment 16 WebKit Commit Bot 2019-02-06 19:13:18 PST
All reviewed patches have been landed.  Closing bug.
Comment 17 Simon Fraser (smfr) 2019-02-06 23:06:37 PST
This was rolled out in https://trac.webkit.org/changeset/241113/webkit
Comment 18 Tim Horton 2019-02-08 23:30:09 PST
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.
Comment 19 Benjamin Poulain 2019-02-11 21:10:45 PST
Reopening, the optimization was reverted.
Comment 20 Benjamin Poulain 2019-02-11 21:38:31 PST
Created attachment 361766 [details]
Patch
Comment 21 Build Bot 2019-02-11 23:27:36 PST
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
Comment 22 Build Bot 2019-02-11 23:27:37 PST
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
Comment 23 Benjamin Poulain 2019-02-12 00:26:57 PST
Created attachment 361777 [details]
Patch
Comment 24 Geoffrey Garen 2019-02-12 18:46:48 PST
Comment on attachment 361777 [details]
Patch

r=me
Comment 25 Benjamin Poulain 2019-02-13 00:15:46 PST
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 26 WebKit Commit Bot 2019-02-13 00:41:27 PST
Comment on attachment 361777 [details]
Patch

Clearing flags on attachment: 361777

Committed r241351: <https://trac.webkit.org/changeset/241351>
Comment 27 WebKit Commit Bot 2019-02-13 00:41:29 PST
All reviewed patches have been landed.  Closing bug.