WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(5.95 KB, patch)
2019-07-17 13:10 PDT
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Chris Dumez
Comment 1
2019-07-16 19:57:30 PDT
Created
attachment 374273
[details]
Patch
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
Created
attachment 374320
[details]
Patch
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
<
rdar://problem/53218922
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug