RESOLVED FIXED 199847
Avoid unnecessary copy of monitors under DisplayRefreshMonitorManager::displayWasUpdated()
https://bugs.webkit.org/show_bug.cgi?id=199847
Summary Avoid unnecessary copy of monitors under DisplayRefreshMonitorManager::displa...
Chris Dumez
Reported 2019-07-16 19:51:43 PDT
Avoid unnecessary copy of monitors under DisplayRefreshMonitorManager::displayWasUpdated().
Attachments
Patch (5.93 KB, patch)
2019-07-16 19:57 PDT, Chris Dumez
no flags
Patch (5.95 KB, patch)
2019-07-17 13:10 PDT, Chris Dumez
no flags
Chris Dumez
Comment 1 2019-07-16 19:57:30 PDT
Per Arne Vollan
Comment 2 2019-07-17 09:06:39 PDT
I thought there could be several monitors of the same display in WebCore, but perhaps I am mistaken?
Chris Dumez
Comment 3 2019-07-17 09:16:35 PDT
(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.
Said Abou-Hallawa
Comment 4 2019-07-17 09:24:02 PDT
(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.
Said Abou-Hallawa
Comment 5 2019-07-17 12:16:00 PDT
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()?
Chris Dumez
Comment 6 2019-07-17 12:19:35 PDT
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.
Said Abou-Hallawa
Comment 7 2019-07-17 12:50:21 PDT
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().
Chris Dumez
Comment 8 2019-07-17 13:08:06 PDT
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.
Chris Dumez
Comment 9 2019-07-17 13:10:15 PDT
WebKit Commit Bot
Comment 10 2019-07-17 13:43:28 PDT
Comment on attachment 374320 [details] Patch Clearing flags on attachment: 374320 Committed r247534: <https://trac.webkit.org/changeset/247534>
WebKit Commit Bot
Comment 11 2019-07-17 13:43:29 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 12 2019-07-17 13:44:19 PDT
Note You need to log in before you can comment on or make changes to this bug.