Bug 216195

Summary: Move lazy DisplayLink tear down logic from the WebProcess to the UIProcess
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebKit2Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ggaren, nham, simon.fraser, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=181396
https://bugs.webkit.org/show_bug.cgi?id=216296
Bug Depends on: 216251    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Chris Dumez 2020-09-04 15:23:59 PDT
Move lazy DisplayLink tear down logic from the WebProcess to the UIProcess, now that the DisplayLink has been moved to the UIProcess due to sandboxing.
After a DisplayLink no longer has any clients, we keep it firing up to 20 times without any clients in case a new client gets added shortly after. The idea was to avoid killing and respawning too many threads when adding and removing clients in quick succession. However, now that the DisplayLink lives in the UIProcess side and send IPC to the WebProcesses every time it fires, it makes a lot more sense to implement this logic in the UIProcess side, to avoid sending unnecessary IPC to processes that do not care about it.
Comment 1 Chris Dumez 2020-09-04 15:26:34 PDT
Created attachment 408033 [details]
Patch
Comment 2 EWS 2020-09-04 16:30:34 PDT
Committed r266645: <https://trac.webkit.org/changeset/266645>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 408033 [details].
Comment 3 Radar WebKit Bug Importer 2020-09-04 16:31:15 PDT
<rdar://problem/68371342>
Comment 4 WebKit Commit Bot 2020-09-07 13:54:10 PDT
Re-opened since this is blocked by bug 216251
Comment 5 Chris Dumez 2020-09-08 10:15:04 PDT
Created attachment 408238 [details]
Patch
Comment 6 Geoffrey Garen 2020-09-08 15:40:35 PDT
Comment on attachment 408238 [details]
Patch

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

> Source/WebKit/UIProcess/mac/DisplayLink.cpp:36
> +constexpr unsigned maxFireCountWithObservers { 20 };

I know I'm late to the party, but shouldn't this be maxFireCountWithoutObservers (adding 'out')?
Comment 7 Chris Dumez 2020-09-08 15:45:44 PDT
(In reply to Geoffrey Garen from comment #6)
> Comment on attachment 408238 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=408238&action=review
> 
> > Source/WebKit/UIProcess/mac/DisplayLink.cpp:36
> > +constexpr unsigned maxFireCountWithObservers { 20 };
> 
> I know I'm late to the party, but shouldn't this be
> maxFireCountWithoutObservers (adding 'out')?

You're totally right :) Also, you're not late since the patch has not been re-landed yet.
Comment 8 Chris Dumez 2020-09-08 15:47:19 PDT
Created attachment 408276 [details]
Patch
Comment 9 Chris Dumez 2020-09-08 18:08:46 PDT
(In reply to Chris Dumez from comment #8)
> Created attachment 408276 [details]
> Patch

Waiting on MemBuster perf results.
Comment 10 Chris Dumez 2020-09-08 18:34:07 PDT
(In reply to Chris Dumez from comment #9)
> (In reply to Chris Dumez from comment #8)
> > Created attachment 408276 [details]
> > Patch
> 
> Waiting on MemBuster perf results.

I meant MotionMark results.
Comment 11 Chris Dumez 2020-09-09 10:54:38 PDT
Created attachment 408344 [details]
Patch
Comment 12 Chris Dumez 2020-09-09 10:55:08 PDT
Confirmed as neutral on MotionMark! Ready for review.
Comment 13 Simon Fraser (smfr) 2020-09-09 11:04:13 PDT
Comment on attachment 408344 [details]
Patch

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

> Source/WebKit/UIProcess/mac/DisplayLink.cpp:79
> +    LockHolder locker(m_observersLock);

Do we need to hold this lock across the call to CVDisplayLinkStart() below?

> Source/WebKit/UIProcess/mac/DisplayLink.cpp:135
> +    if (displayLink->m_observers.isEmpty()) {
> +        if (++(displayLink->m_fireCountWithoutObservers) >= maxFireCountWithoutObservers)
> +            CVDisplayLinkStop(displayLink->m_displayLink);
> +        return kCVReturnSuccess;
> +    }
> +    displayLink->m_fireCountWithoutObservers = 0;
> +
>      for (auto& connection : displayLink->m_observers.keys()) {

Lots of displayLink-> here. Would be cleaner as a member function on DisplayLink.
Comment 14 Chris Dumez 2020-09-09 11:22:30 PDT
Created attachment 408350 [details]
Patch
Comment 15 EWS 2020-09-09 12:47:58 PDT
Committed r266797: <https://trac.webkit.org/changeset/266797>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 408350 [details].