Avoid unnecessary copy of monitors under DisplayRefreshMonitorManager::displayWasUpdated().
Created attachment 374273 [details] Patch
I thought there could be several monitors of the same display in WebCore, but perhaps I am mistaken?
(In reply to Per Arne Vollan from comment #2) > I thought there could be several monitors of the same display in WebCore, > but perhaps I am mistaken? If you look at DisplayRefreshMonitorManager::createMonitorForClient(), I do not see how this could be.
(In reply to Per Arne Vollan from comment #2) > I thought there could be several monitors of the same display in WebCore, > but perhaps I am mistaken? There is one DisplayRefreshMonitor for every physical display. But there might be multiple DisplayRefreshMonitorClients for each DisplayRefreshMonitor. The DisplayRefreshMonitor and all its clients have the same PlatformDisplayID.
Comment on attachment 374273 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374273&action=review > Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.cpp:84 > + auto index = findMonitorForDisplay(clientDisplayID); > + if (index == notFound) > return; > + RefPtr<DisplayRefreshMonitor> monitor = m_monitors[index].monitor; > + if (monitor->removeClient(client)) { > + if (!monitor->hasClients()) > + m_monitors.remove(index); I think using this pattern is better: if (auto* existingMonitor = monitorForDisplay(clientDisplayID)) { ... } > Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.cpp:128 > + auto* monitor = monitorForDisplay(displayID); > + if (monitor && monitor->hasRequestedRefreshCallback()) > + monitor->displayLinkFired(); Ditto. > Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.cpp:136 > +size_t DisplayRefreshMonitorManager::findMonitorForDisplay(PlatformDisplayID displayID) const > +{ > + return m_monitors.findMatching([&](auto& monitorWrapper) { > + return monitorWrapper.monitor->displayID() == displayID; > }); > - for (auto& monitor : monitors) { > - if (displayID == monitor->displayID() && monitor->hasRequestedRefreshCallback()) > - monitor->displayLinkFired(); > - } > +} I think this function should is not needed. > Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.cpp:138 > +DisplayRefreshMonitor* DisplayRefreshMonitorManager::monitorForDisplay(PlatformDisplayID displayID) const monitorForDisplayID() or finMointorForDisplayID()?
Comment on attachment 374273 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374273&action=review >> Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.cpp:84 >> + m_monitors.remove(index); > > I think using this pattern is better: > > if (auto* existingMonitor = monitorForDisplay(clientDisplayID)) { > ... > } This adds one more level of nesting here (3 instead of 2) and does not really reduce the scope of the monitor variable since there is nothing after the if (). >> Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.cpp:128 >> + monitor->displayLinkFired(); > > Ditto. Ditto. >> Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.cpp:136 >> +} > > I think this function should is not needed. It is used in 2 places so what is your suggestion? Duplicating the code? >> Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.cpp:138 >> +DisplayRefreshMonitor* DisplayRefreshMonitorManager::monitorForDisplay(PlatformDisplayID displayID) const > > monitorForDisplayID() or finMointorForDisplayID()? I think we monitorForDisplayID() is our common pattern for such getters. I like find*() for something returning an index since it matches our Vector API.
Comment on attachment 374273 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374273&action=review >>> Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.cpp:84 >>> + m_monitors.remove(index); >> >> I think using this pattern is better: >> >> if (auto* existingMonitor = monitorForDisplay(clientDisplayID)) { >> ... >> } > > This adds one more level of nesting here (3 instead of 2) and does not really reduce the scope of the monitor variable since there is nothing after the if (). Okay. I did not notice that we need the 'index' to remove the monitor from the Vector m_monitors. So I was suggesting using monitorForDisplay() instead of findMonitorForDisplay(). >>> Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.cpp:136 >>> +} >> >> I think this function should is not needed. > > It is used in 2 places so what is your suggestion? Duplicating the code? My comment was based on the above suggestion in which I was suggesting using monitorForDisplay() instead of findMonitorForDisplay(). >>> Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.cpp:138 >>> +DisplayRefreshMonitor* DisplayRefreshMonitorManager::monitorForDisplay(PlatformDisplayID displayID) const >> >> monitorForDisplayID() or finMointorForDisplayID()? > > I think we monitorForDisplayID() is our common pattern for such getters. I like find*() for something returning an index since it matches our Vector API. Fine I was suggesting: monitorForDisplay() -> monitorForDisplayID().
Comment on attachment 374273 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374273&action=review >>>> Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.cpp:138 >>>> +DisplayRefreshMonitor* DisplayRefreshMonitorManager::monitorForDisplay(PlatformDisplayID displayID) const >>> >>> monitorForDisplayID() or finMointorForDisplayID()? >> >> I think we monitorForDisplayID() is our common pattern for such getters. I like find*() for something returning an index since it matches our Vector API. > > Fine I was suggesting: monitorForDisplay() -> monitorForDisplayID(). Oh, I failed to notice. Yes, this looks better. Will fix before landing, thanks.
Created attachment 374320 [details] Patch
Comment on attachment 374320 [details] Patch Clearing flags on attachment: 374320 Committed r247534: <https://trac.webkit.org/changeset/247534>
All reviewed patches have been landed. Closing bug.
<rdar://problem/53218922>