- Revision
- 117744
- Author
- [email protected]
- Date
- 2012-05-21 01:14:46 -0700 (Mon, 21 May 2012)
Log Message
REGRESSION(r117501): IconDatabase asserts on startup in synchronousIconForPageURL().
<http://webkit.org/b/86935>
<rdar://problem/11480012>
Reviewed by Anders Carlsson.
Source/WebCore:
- Correctly set m_retainOrReleaseIconRequested to true in retainIconForPageURL().
This was causing the assertions, as we would end up doing nothing until the first
icon release request came in.
- Require that m_urlsToRetainOrReleaseLock be held when accessing m_retainOrReleaseIconRequested.
This removes a possible race condition in double checked locking.
- Swap over the retain/release work queues while holding m_urlsToRetainOrReleaseLock
and release it right away to avoid sitting on the lock while updating the database.
* loader/icon/IconDatabase.cpp:
(WebCore::IconDatabase::synchronousIconForPageURL):
(WebCore::IconDatabase::retainIconForPageURL):
(WebCore::IconDatabase::releaseIconForPageURL):
(WebCore::IconDatabase::retainedPageURLCount):
(WebCore::IconDatabase::performURLImport):
(WebCore::IconDatabase::syncThreadMainLoop):
(WebCore::IconDatabase::performPendingRetainAndReleaseOperations):
* loader/icon/IconDatabase.h:
(IconDatabase):
Source/WTF:
Added a swap() to HashCountedSet.
* wtf/HashCountedSet.h:
(HashCountedSet::swap):
Modified Paths
Diff
Modified: trunk/Source/WTF/ChangeLog (117743 => 117744)
--- trunk/Source/WTF/ChangeLog 2012-05-21 08:08:14 UTC (rev 117743)
+++ trunk/Source/WTF/ChangeLog 2012-05-21 08:14:46 UTC (rev 117744)
@@ -1,3 +1,16 @@
+2012-05-21 Andreas Kling <[email protected]>
+
+ REGRESSION(r117501): IconDatabase asserts on startup in synchronousIconForPageURL().
+ <http://webkit.org/b/86935>
+ <rdar://problem/11480012>
+
+ Reviewed by Anders Carlsson.
+
+ Added a swap() to HashCountedSet.
+
+ * wtf/HashCountedSet.h:
+ (HashCountedSet::swap):
+
2012-05-16 Geoffrey Garen <[email protected]>
This is not a joke: 3.7X speedup from removing a call to sleep
Modified: trunk/Source/WTF/wtf/HashCountedSet.h (117743 => 117744)
--- trunk/Source/WTF/wtf/HashCountedSet.h 2012-05-21 08:08:14 UTC (rev 117743)
+++ trunk/Source/WTF/wtf/HashCountedSet.h 2012-05-21 08:14:46 UTC (rev 117744)
@@ -39,6 +39,8 @@
typedef typename ImplType::AddResult AddResult;
HashCountedSet() {}
+
+ void swap(HashCountedSet&);
int size() const;
int capacity() const;
@@ -75,6 +77,12 @@
private:
ImplType m_impl;
};
+
+ template<typename Value, typename HashFunctions, typename Traits>
+ inline void HashCountedSet<Value, HashFunctions, Traits>::swap(HashCountedSet& other)
+ {
+ m_impl.swap(other.m_impl);
+ }
template<typename Value, typename HashFunctions, typename Traits>
inline int HashCountedSet<Value, HashFunctions, Traits>::size() const
Modified: trunk/Source/WebCore/ChangeLog (117743 => 117744)
--- trunk/Source/WebCore/ChangeLog 2012-05-21 08:08:14 UTC (rev 117743)
+++ trunk/Source/WebCore/ChangeLog 2012-05-21 08:14:46 UTC (rev 117744)
@@ -1,3 +1,32 @@
+2012-05-18 Andreas Kling <[email protected]>
+
+ REGRESSION(r117501): IconDatabase asserts on startup in synchronousIconForPageURL().
+ <http://webkit.org/b/86935>
+ <rdar://problem/11480012>
+
+ Reviewed by Anders Carlsson.
+
+ - Correctly set m_retainOrReleaseIconRequested to true in retainIconForPageURL().
+ This was causing the assertions, as we would end up doing nothing until the first
+ icon release request came in.
+
+ - Require that m_urlsToRetainOrReleaseLock be held when accessing m_retainOrReleaseIconRequested.
+ This removes a possible race condition in double checked locking.
+
+ - Swap over the retain/release work queues while holding m_urlsToRetainOrReleaseLock
+ and release it right away to avoid sitting on the lock while updating the database.
+
+ * loader/icon/IconDatabase.cpp:
+ (WebCore::IconDatabase::synchronousIconForPageURL):
+ (WebCore::IconDatabase::retainIconForPageURL):
+ (WebCore::IconDatabase::releaseIconForPageURL):
+ (WebCore::IconDatabase::retainedPageURLCount):
+ (WebCore::IconDatabase::performURLImport):
+ (WebCore::IconDatabase::syncThreadMainLoop):
+ (WebCore::IconDatabase::performPendingRetainAndReleaseOperations):
+ * loader/icon/IconDatabase.h:
+ (IconDatabase):
+
2012-05-21 Kent Tamura <[email protected]>
Unreviewed, build fix.
Modified: trunk/Source/WebCore/loader/icon/IconDatabase.cpp (117743 => 117744)
--- trunk/Source/WebCore/loader/icon/IconDatabase.cpp 2012-05-21 08:08:14 UTC (rev 117743)
+++ trunk/Source/WebCore/loader/icon/IconDatabase.cpp 2012-05-21 08:14:46 UTC (rev 117744)
@@ -223,8 +223,7 @@
MutexLocker locker(m_urlAndIconLock);
- if (m_retainOrReleaseIconRequested)
- performPendingRetainAndReleaseOperations();
+ performPendingRetainAndReleaseOperations();
String pageURLCopy; // Creates a null string for easy testing
@@ -398,9 +397,13 @@
if (!isEnabled() || !documentCanHaveIcon(pageURL))
return;
-
- MutexLocker locker(m_urlsToRetainOrReleaseLock);
- m_urlsToRetain.add(pageURL);
+
+ {
+ MutexLocker locker(m_urlsToRetainOrReleaseLock);
+ m_urlsToRetain.add(pageURL);
+ m_retainOrReleaseIconRequested = true;
+ }
+
scheduleOrDeferSyncTimer();
}
@@ -448,9 +451,11 @@
if (!isEnabled() || !documentCanHaveIcon(pageURL))
return;
- MutexLocker locker(m_urlsToRetainOrReleaseLock);
- m_urlsToRelease.add(pageURL);
- m_retainOrReleaseIconRequested = true;
+ {
+ MutexLocker locker(m_urlsToRetainOrReleaseLock);
+ m_urlsToRelease.add(pageURL);
+ m_retainOrReleaseIconRequested = true;
+ }
scheduleOrDeferSyncTimer();
}
@@ -744,10 +749,7 @@
size_t IconDatabase::retainedPageURLCount()
{
MutexLocker locker(m_urlAndIconLock);
-
- if (m_retainOrReleaseIconRequested)
- performPendingRetainAndReleaseOperations();
-
+ performPendingRetainAndReleaseOperations();
return m_retainedPageURLs.size();
}
@@ -1334,8 +1336,7 @@
{
MutexLocker locker(m_urlAndIconLock);
- if (m_retainOrReleaseIconRequested)
- performPendingRetainAndReleaseOperations();
+ performPendingRetainAndReleaseOperations();
for (unsigned i = 0; i < urls.size(); ++i) {
if (!m_retainedPageURLs.contains(urls[i])) {
@@ -1418,12 +1419,9 @@
if (m_threadTerminationRequested)
break;
- if (m_retainOrReleaseIconRequested) {
+ {
MutexLocker locker(m_urlAndIconLock);
- // Previous flag check was done outside of the lock and flag could be changed by another thread.
- // Do not move mutex outside to avoid unnecessary locking on every loop, but recheck the flag under mutex.
- if (m_retainOrReleaseIconRequested)
- performPendingRetainAndReleaseOperations();
+ performPendingRetainAndReleaseOperations();
}
bool didAnyWork = true;
@@ -1511,21 +1509,29 @@
void IconDatabase::performPendingRetainAndReleaseOperations()
{
- ASSERT(m_retainOrReleaseIconRequested);
-
// NOTE: The caller is assumed to hold m_urlAndIconLock.
ASSERT(!m_urlAndIconLock.tryLock());
- MutexLocker vectorLocker(m_urlsToRetainOrReleaseLock);
+ HashCountedSet<String> toRetain;
+ HashCountedSet<String> toRelease;
- for (HashCountedSet<String>::const_iterator it = m_urlsToRetain.begin(), end = m_urlsToRetain.end(); it != end; ++it)
+ {
+ MutexLocker pendingWorkLocker(m_urlsToRetainOrReleaseLock);
+ if (!m_retainOrReleaseIconRequested)
+ return;
+
+ // Make a copy of the URLs to retain and/or release so we can release m_urlsToRetainOrReleaseLock ASAP.
+ // Holding m_urlAndIconLock protects this function from being re-entered.
+
+ toRetain.swap(m_urlsToRetain);
+ toRelease.swap(m_urlsToRelease);
+ m_retainOrReleaseIconRequested = false;
+ }
+
+ for (HashCountedSet<String>::const_iterator it = toRetain.begin(), end = toRetain.end(); it != end; ++it)
performRetainIconForPageURL(it->first, it->second);
- for (HashCountedSet<String>::const_iterator it = m_urlsToRelease.begin(), end = m_urlsToRelease.end(); it != end; ++it)
+ for (HashCountedSet<String>::const_iterator it = toRelease.begin(), end = toRelease.end(); it != end; ++it)
performReleaseIconForPageURL(it->first, it->second);
-
- m_urlsToRetain.clear();
- m_urlsToRelease.clear();
- m_retainOrReleaseIconRequested = false;
}
bool IconDatabase::readFromDatabase()
Modified: trunk/Source/WebCore/loader/icon/IconDatabase.h (117743 => 117744)
--- trunk/Source/WebCore/loader/icon/IconDatabase.h 2012-05-21 08:08:14 UTC (rev 117743)
+++ trunk/Source/WebCore/loader/icon/IconDatabase.h 2012-05-21 08:14:46 UTC (rev 117744)
@@ -165,7 +165,6 @@
bool m_iconURLImportComplete;
bool m_syncThreadHasWorkToDo;
bool m_disabledSuddenTerminationForSyncThread;
- bool m_retainOrReleaseIconRequested;
Mutex m_urlAndIconLock;
// Holding m_urlAndIconLock is required when accessing any of the following data structures or the objects they contain
@@ -188,6 +187,7 @@
// Holding m_urlsToRetainOrReleaseLock is required when accessing any of the following data structures.
HashCountedSet<String> m_urlsToRetain;
HashCountedSet<String> m_urlsToRelease;
+ bool m_retainOrReleaseIconRequested;
// *** Sync Thread Only ***
public:
