Bug 216195 – Move lazy DisplayLink tear down logic from the WebProcess to the UIProcess

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 408033 [details]
Patch

[EWS]

Committed r266645: <https://trac.webkit.org/changeset/266645>

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

[Radar WebKit Bug Importer]

<rdar://problem/68371342>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 216251

[Chris Dumez]

Created attachment 408238 [details]
Patch

[Geoffrey Garen]

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')?

[Chris Dumez]

(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.

[Chris Dumez]

Created attachment 408276 [details]
Patch

[Chris Dumez]

(In reply to Chris Dumez from comment #8)
> Created attachment 408276 [details]
> Patch

Waiting on MemBuster perf results.

[Chris Dumez]

(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.

[Chris Dumez]

Created attachment 408344 [details]
Patch

[Chris Dumez]

Confirmed as neutral on MotionMark! Ready for review.

[Simon Fraser (smfr)]

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.

[Chris Dumez]

Created attachment 408350 [details]
Patch

[EWS]

Committed r266797: <https://trac.webkit.org/changeset/266797>

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