Many CVDisplayLink threads created and destroyed while watching YouTube video
YouTube's media controls (and the default media controls on Mac & iOS) appear to use requestAnimationFrame() to implement layout changes during playback. The most common event which triggers layout changes is the 'timeupdate' event, which according to the HTML spec, should fire every 15 to 250ms. WebKit picks the slowest update value, 250ms. The DisplayRefreshMonitor implements the timer which triggers rAF, and it will currently destroy the underlying platform object which handles the display refresh notification if there are no rAF clients for 10 consecutive display refresh intervals, or: 10 / 60hz = 166ms. So a naive client that does something like: video.addEventListener('timeupdate', event => { doUpdate(event); }) will cause the platform rAF object to be created and destroyed 4x a second. Lets update the constant in DisplayRefreshMonitor to, e.g., 22 consecutive display intervals without a client (or 366ms), so that clients of "slow" firing consistently events like 'timeupdate' and 'progress' don't inadvertently cause a lot of tear-down/bring-up overhead.
Created attachment 330739 [details] Patch
Comment on attachment 330739 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=330739&action=review > Source/WebCore/platform/graphics/DisplayRefreshMonitor.h:59 > + const int maxInactiveFireCount = 20; I wonder if we should make this a Seconds and do it based on time, rather than fire count? > Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.cpp:110 > + LOG(RequestAnimationFrame, "DisplayRefreshMonitorManager::displayDidRefresh() - destroying monitor %p", &monitor); You should log when the rAF fires too.
(In reply to Simon Fraser (smfr) from comment #3) > Comment on attachment 330739 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=330739&action=review > > > Source/WebCore/platform/graphics/DisplayRefreshMonitor.h:59 > > + const int maxInactiveFireCount = 20; > > I wonder if we should make this a Seconds and do it based on time, rather > than fire count? That would definitely future proof us against future non-60fps monitor refresh rates. I'll file a follow-up bug. > > Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.cpp:110 > > + LOG(RequestAnimationFrame, "DisplayRefreshMonitorManager::displayDidRefresh() - destroying monitor %p", &monitor); > > You should log when the rAF fires too.
Created attachment 330749 [details] Patch for landing
(In reply to Jer Noble from comment #4) > (In reply to Simon Fraser (smfr) from comment #3) > > Comment on attachment 330739 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=330739&action=review > > > > > Source/WebCore/platform/graphics/DisplayRefreshMonitor.h:59 > > > + const int maxInactiveFireCount = 20; > > > > I wonder if we should make this a Seconds and do it based on time, rather > > than fire count? > > That would definitely future proof us against future non-60fps monitor > refresh rates. I'll file a follow-up bug. Filed bug# 181411.
Comment on attachment 330749 [details] Patch for landing Rejecting attachment 330749 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 330749, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!. Full output: http://webkit-queues.webkit.org/results/5980111
Created attachment 330838 [details] Patch for landing
Comment on attachment 330838 [details] Patch for landing Clearing flags on attachment: 330838 Committed r226664: <https://trac.webkit.org/changeset/226664>
All reviewed patches have been landed. Closing bug.
<rdar://problem/36388728>