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.
Created attachment 408033 [details] Patch
Committed r266645: <https://trac.webkit.org/changeset/266645> All reviewed patches have been landed. Closing bug and clearing flags on attachment 408033 [details].
<rdar://problem/68371342>
Re-opened since this is blocked by bug 216251
Created attachment 408238 [details] Patch
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')?
(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.
Created attachment 408276 [details] Patch
(In reply to Chris Dumez from comment #8) > Created attachment 408276 [details] > Patch Waiting on MemBuster perf results.
(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.
Created attachment 408344 [details] Patch
Confirmed as neutral on MotionMark! Ready for review.
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.
Created attachment 408350 [details] Patch
Committed r266797: <https://trac.webkit.org/changeset/266797> All reviewed patches have been landed. Closing bug and clearing flags on attachment 408350 [details].