Bug 194003 – Responsiveness timers are too expensive for frequent events

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Benjamin Poulain]

<rdar://problem/47570443> Responsiveness timers are too expensive for frequent events

[Benjamin Poulain]

Created attachment 360549 [details]
Patch

[Geoffrey Garen]

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

[Geoffrey Garen]

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.

[Benjamin Poulain]

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.

[Benjamin Poulain]

Created attachment 360676 [details]
Patch

[Benjamin Poulain]

Tested by:
-Disable "platformIsBeingDebugged"
-Use lldb to stop the WebProcess
-Click on the page.
-Type in a text field
-Scroll with wheel events

[WebKit Commit Bot]

Comment on attachment 360676 [details]
Patch

Clearing flags on attachment: 360676

Committed r240759: <https://trac.webkit.org/changeset/240759>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/47692525>

[Simon Fraser (smfr)]

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?

[Benjamin Poulain]

(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 :)

[Benjamin Poulain]

Reopening to attach new patch.

[Benjamin Poulain]

Created attachment 361354 [details]
Patch

[EWS Watchlist]

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.

[WebKit Commit Bot]

Comment on attachment 361354 [details]
Patch

Clearing flags on attachment: 361354

Committed r241113: <https://trac.webkit.org/changeset/241113>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

This was rolled out in https://trac.webkit.org/changeset/241113/webkit

[Tim Horton]

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.

[Benjamin Poulain]

Reopening, the optimization was reverted.

[Benjamin Poulain]

Created attachment 361766 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Benjamin Poulain]

Created attachment 361777 [details]
Patch

[Geoffrey Garen]

Comment on attachment 361777 [details]
Patch

r=me

[Benjamin Poulain]

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!

[WebKit Commit Bot]

Comment on attachment 361777 [details]
Patch

Clearing flags on attachment: 361777

Committed r241351: <https://trac.webkit.org/changeset/241351>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.