Changeset 179850 in webkit

Timestamp:

Feb 9, 2015, 3:39:41 PM (12 years ago)

Author:

timothy_horton@apple.com

Message:

Avoid using a HashMap for DisplayRefreshMonitorManager, which rarely has more than one item
​https://bugs.webkit.org/show_bug.cgi?id=141353

Reviewed by Anders Carlsson.

No new tests, because there's no behavior change.

• platform/graphics/DisplayRefreshMonitorManager.cpp:

(WebCore::DisplayRefreshMonitorManager::ensureMonitorForClient):
(WebCore::DisplayRefreshMonitorManager::unregisterClient):
(WebCore::DisplayRefreshMonitorManager::displayDidRefresh):

• platform/graphics/DisplayRefreshMonitorManager.h:

Use a Vector of RefPtr<DisplayRefreshMonitor> instead of a HashMap
from uint64_t to RefPtr<DisplayRefreshMonitor>. There's usually only one
display, so there's usually only one DisplayRefreshMonitor. Linear search
on the Vector will be faster than the hash lookup in all conceivable cases.
This also avoids the situation mentioned in the comments in DisplayRefreshMonitorManager.h
where we don't know enough about PlatformDisplayID to safely hash it.

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• platform/graphics/DisplayRefreshMonitorManager.cpp (modified) (3 diffs)
• platform/graphics/DisplayRefreshMonitorManager.h (modified) (2 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2015-02-09  Timothy Horton  <timothy_horton@apple.com>
+
+        Avoid using a HashMap for DisplayRefreshMonitorManager, which rarely has more than one item
+        https://bugs.webkit.org/show_bug.cgi?id=141353
+
+        Reviewed by Anders Carlsson.
+
+        No new tests, because there's no behavior change.
+
+        * platform/graphics/DisplayRefreshMonitorManager.cpp:
+        (WebCore::DisplayRefreshMonitorManager::ensureMonitorForClient):
+        (WebCore::DisplayRefreshMonitorManager::unregisterClient):
+        (WebCore::DisplayRefreshMonitorManager::displayDidRefresh):
+        * platform/graphics/DisplayRefreshMonitorManager.h:
+        Use a Vector of RefPtr<DisplayRefreshMonitor> instead of a HashMap
+        from uint64_t to RefPtr<DisplayRefreshMonitor>. There's usually only one
+        display, so there's usually only one DisplayRefreshMonitor. Linear search
+        on the Vector will be faster than the hash lookup in all conceivable cases.
+        This also avoids the situation mentioned in the comments in DisplayRefreshMonitorManager.h
+        where we don't know enough about PlatformDisplayID to safely hash it.
+
 2015-02-09  Jer Noble  <jer.noble@apple.com>
 

trunk/Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.cpp

@@ -47 +47 @@
 DisplayRefreshMonitor* DisplayRefreshMonitorManager::ensureMonitorForClient(DisplayRefreshMonitorClient* client)
 {
-    DisplayRefreshMonitorMap::iterator it = m_monitors.find(client->displayID());
-    if (it == m_monitors.end()) {
-        RefPtr<DisplayRefreshMonitor> monitor = DisplayRefreshMonitor::create(client);
+    PlatformDisplayID clientDisplayID = client->displayID();
+    for (const RefPtr<DisplayRefreshMonitor>& monitor : m_monitors) {
+        if (monitor->displayID() != clientDisplayID)
+            continue;
         monitor->addClient(client);
-        DisplayRefreshMonitor* result = monitor.get();
-        m_monitors.add(client->displayID(), monitor.release());
-        return result;
+        return monitor.get();
     }
-    it->value->addClient(client);
-    return it->value.get();
+
+    RefPtr<DisplayRefreshMonitor> monitor = DisplayRefreshMonitor::create(client);
+    monitor->addClient(client);
+    DisplayRefreshMonitor* result = monitor.get();
+    m_monitors.append(monitor.release());
+    return result;
 }
 
@@ -72 +75 @@
         return;
 
-    DisplayRefreshMonitorMap::iterator it = m_monitors.find(client->displayID());
-    if (it == m_monitors.end())
+    PlatformDisplayID clientDisplayID = client->displayID();
+    for (size_t i = 0; i < m_monitors.size(); ++i) {
+        RefPtr<DisplayRefreshMonitor> monitor = m_monitors[i];
+        if (monitor->displayID() != clientDisplayID)
+            continue;
+        if (monitor->removeClient(client)) {
+            if (!monitor->hasClients())
+                m_monitors.remove(i);
+        }
         return;
-
-    DisplayRefreshMonitor* monitor = it->value.get();
-    if (monitor->removeClient(client)) {
-        if (!monitor->hasClients())
-            m_monitors.remove(it);
     }
 }
@@ -96 +101 @@
 void DisplayRefreshMonitorManager::displayDidRefresh(DisplayRefreshMonitor* monitor)
 {
-    if (monitor->shouldBeTerminated()) {
-        ASSERT(m_monitors.contains(monitor->displayID()));
-        m_monitors.remove(monitor->displayID());
-    }
+    if (!monitor->shouldBeTerminated())
+        return;
+
+    size_t monitorIndex = m_monitors.find(monitor);
+    ASSERT(monitorIndex != notFound);
+    m_monitors.remove(monitorIndex);
 }
 

trunk/Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.h

@@ -31 +31 @@
 #include "DisplayRefreshMonitor.h"
 #include "PlatformScreen.h"
-#include <wtf/HashMap.h>
 #include <wtf/NeverDestroyed.h>
 #include <wtf/RefPtr.h>
+#include <wtf/Vector.h>
 
 namespace WebCore {
@@ -57 +57 @@
     DisplayRefreshMonitor* ensureMonitorForClient(DisplayRefreshMonitorClient*);
 
-    // We know nothing about the values of PlatformDisplayIDs, so use UnsignedWithZeroKeyHashTraits.
-    // FIXME: Since we know nothing about these values, this is not sufficient.
-    // Even with UnsignedWithZeroKeyHashTraits, there are still two special values used for empty and deleted hash table slots.
-    typedef HashMap<uint64_t, RefPtr<DisplayRefreshMonitor>, WTF::IntHash<uint64_t>, WTF::UnsignedWithZeroKeyHashTraits<uint64_t>> DisplayRefreshMonitorMap;
-    DisplayRefreshMonitorMap m_monitors;
+    Vector<RefPtr<DisplayRefreshMonitor>> m_monitors;
 };