Bug 199847 – Avoid unnecessary copy of monitors under DisplayRefreshMonitorManager::displayWasUpdated()

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Chris Dumez]

Avoid unnecessary copy of monitors under DisplayRefreshMonitorManager::displayWasUpdated().

[Chris Dumez]

Created attachment 374273 [details]
Patch

[Per Arne Vollan]

I thought there could be several monitors of the same display in WebCore, but perhaps I am mistaken?

[Chris Dumez]

(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]

(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 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 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 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 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]

Created attachment 374320 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 374320 [details]
Patch

Clearing flags on attachment: 374320

Committed r247534: <https://trac.webkit.org/changeset/247534>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/53218922>