Modified: trunk/Source/WebCore/ChangeLog (117500 => 117501)
--- trunk/Source/WebCore/ChangeLog 2012-05-17 21:46:02 UTC (rev 117500)
+++ trunk/Source/WebCore/ChangeLog 2012-05-17 21:49:43 UTC (rev 117501)
@@ -1,3 +1,46 @@
+2012-05-15 Andreas Kling <kl...@webkit.org>
+
+ IconDatabase: Move icon retain/release off of the main thread.
+ <http://webkit.org/b/85799>
+ <rdar://problem/9507113>
+
+ Reviewed by Brady Eidson.
+
+ Batch up the retain/release operations and execute them as part of the sync thread loop.
+ The batch execution is guarded by a new mutex (m_urlsToRetainOrReleaseLock.)
+ This avoids blocking the main thread on m_urlAndIconLock for basic retain/release.
+
+ There is one exception; if there are pending retain/release operations in synchronousIconForPageURL,
+ it will acquire the lock and flush the operations.
+
+ There should be no behavior change, this is only meant to reduce lock contention.
+
+ * loader/icon/PageURLRecord.h:
+ (WebCore::PageURLRecord::retain):
+ (WebCore::PageURLRecord::release):
+
+ Added a 'count' argument to these so we can batch up the operations in IconDatabase.
+
+ * loader/icon/IconDatabase.h:
+ * loader/icon/IconDatabase.cpp:
+ (WebCore::IconDatabase::performScheduleOrDeferSyncTimer):
+ (WebCore::IconDatabase::performScheduleOrDeferSyncTimerOnMainThread):
+ (WebCore::IconDatabase::scheduleOrDeferSyncTimer):
+
+ Perform the the timer scheduling on the main thread as it can be done on a different
+ thread by way of retainIconForPageURL or releaseIconForPageURL.
+
+ (WebCore::IconDatabase::synchronousIconForPageURL):
+ (WebCore::IconDatabase::retainIconForPageURL):
+ (WebCore::IconDatabase::performRetainIconForPageURL):
+ (WebCore::IconDatabase::releaseIconForPageURL):
+ (WebCore::IconDatabase::performReleaseIconForPageURL):
+ (WebCore::IconDatabase::retainedPageURLCount):
+ (WebCore::IconDatabase::IconDatabase):
+ (WebCore::IconDatabase::performURLImport):
+ (WebCore::IconDatabase::syncThreadMainLoop):
+ (WebCore::IconDatabase::performPendingRetainAndReleaseOperations):
+
2012-05-17 Julien Chaffraix <jchaffr...@webkit.org>
Kill RenderLayer::relativePositionOffset(LayoutUnit& relX, LayoutUnit& relY) and cleanup RenderInline::clippedOverflowRectForRepaint
Modified: trunk/Source/WebCore/loader/icon/IconDatabase.cpp (117500 => 117501)
--- trunk/Source/WebCore/loader/icon/IconDatabase.cpp 2012-05-17 21:46:02 UTC (rev 117500)
+++ trunk/Source/WebCore/loader/icon/IconDatabase.cpp 2012-05-17 21:49:43 UTC (rev 117501)
@@ -222,6 +222,9 @@
return 0;
MutexLocker locker(m_urlAndIconLock);
+
+ if (m_retainOrReleaseIconRequested)
+ performPendingRetainAndReleaseOperations();
String pageURLCopy; // Creates a null string for easy testing
@@ -389,18 +392,20 @@
return m_defaultIconRecord->image(size);
}
+void IconDatabase::retainIconForPageURL(const String& pageURL)
+{
+ ASSERT_NOT_SYNC_THREAD();
-void IconDatabase::retainIconForPageURL(const String& pageURLOriginal)
-{
- ASSERT_NOT_SYNC_THREAD();
-
- // Cannot do anything with pageURLOriginal that would end up storing it without deep copying first
-
- if (!isEnabled() || !documentCanHaveIcon(pageURLOriginal))
+ if (!isEnabled() || !documentCanHaveIcon(pageURL))
return;
- MutexLocker locker(m_urlAndIconLock);
+ MutexLocker locker(m_urlsToRetainOrReleaseLock);
+ m_urlsToRetain.add(pageURL);
+ scheduleOrDeferSyncTimer();
+}
+void IconDatabase::performRetainIconForPageURL(const String& pageURLOriginal, int retainCount)
+{
PageURLRecord* record = m_pageURLToRecordMap.get(pageURLOriginal);
String pageURL;
@@ -412,7 +417,7 @@
m_pageURLToRecordMap.set(pageURL, record);
}
- if (!record->retain()) {
+ if (!record->retain(retainCount)) {
if (pageURL.isNull())
pageURL = pageURLOriginal.isolatedCopy();
@@ -434,17 +439,23 @@
}
}
-void IconDatabase::releaseIconForPageURL(const String& pageURLOriginal)
+void IconDatabase::releaseIconForPageURL(const String& pageURL)
{
ASSERT_NOT_SYNC_THREAD();
// Cannot do anything with pageURLOriginal that would end up storing it without deep copying first
- if (!isEnabled() || !documentCanHaveIcon(pageURLOriginal))
+ if (!isEnabled() || !documentCanHaveIcon(pageURL))
return;
-
- MutexLocker locker(m_urlAndIconLock);
+ MutexLocker locker(m_urlsToRetainOrReleaseLock);
+ m_urlsToRelease.add(pageURL);
+ m_retainOrReleaseIconRequested = true;
+ scheduleOrDeferSyncTimer();
+}
+
+void IconDatabase::performReleaseIconForPageURL(const String& pageURLOriginal, int releaseCount)
+{
// Check if this pageURL is actually retained
if (!m_retainedPageURLs.contains(pageURLOriginal)) {
LOG_ERROR("Attempting to release icon for URL %s which is not retained", urlForLogging(pageURLOriginal).ascii().data());
@@ -458,7 +469,7 @@
ASSERT(pageRecord->retainCount() > 0);
// If it still has a positive retain count, store the new count and bail
- if (pageRecord->release())
+ if (pageRecord->release(releaseCount))
return;
// This pageRecord has now been fully released. Do the appropriate cleanup
@@ -498,9 +509,6 @@
}
delete pageRecord;
-
- if (isOpen())
- scheduleOrDeferSyncTimer();
}
void IconDatabase::setIconDataForIconURL(PassRefPtr<SharedBuffer> dataOriginal, const String& iconURLOriginal)
@@ -736,6 +744,10 @@
size_t IconDatabase::retainedPageURLCount()
{
MutexLocker locker(m_urlAndIconLock);
+
+ if (m_retainOrReleaseIconRequested)
+ performPendingRetainAndReleaseOperations();
+
return m_retainedPageURLs.size();
}
@@ -762,6 +774,7 @@
IconDatabase::IconDatabase()
: m_syncTimer(this, &IconDatabase::syncTimerFired)
, m_syncThreadRunning(false)
+ , m_scheduleOrDeferSyncTimerRequested(false)
, m_isEnabled(false)
, m_privateBrowsingEnabled(false)
, m_threadTerminationRequested(false)
@@ -823,17 +836,30 @@
m_syncCondition.signal();
}
+void IconDatabase::performScheduleOrDeferSyncTimer()
+{
+ m_syncTimer.startOneShot(updateTimerDelay);
+ m_scheduleOrDeferSyncTimerRequested = false;
+}
+
+void IconDatabase::performScheduleOrDeferSyncTimerOnMainThread(void* context)
+{
+ static_cast<IconDatabase*>(context)->performScheduleOrDeferSyncTimer();
+}
+
void IconDatabase::scheduleOrDeferSyncTimer()
{
ASSERT_NOT_SYNC_THREAD();
- if (!m_syncTimer.isActive()) {
- // The following is balanced by the call to enableSuddenTermination in the
- // syncTimerFired function.
- disableSuddenTermination();
- }
+ if (m_scheduleOrDeferSyncTimerRequested)
+ return;
- m_syncTimer.startOneShot(updateTimerDelay);
+ // The following is balanced by the call to enableSuddenTermination in the
+ // syncTimerFired function.
+ disableSuddenTermination();
+
+ m_scheduleOrDeferSyncTimerRequested = true;
+ callOnMainThread(performScheduleOrDeferSyncTimerOnMainThread, this);
}
void IconDatabase::syncTimerFired(Timer<IconDatabase>*)
@@ -1306,7 +1332,10 @@
// Keep a set of ones that are retained and pending notification
{
MutexLocker locker(m_urlAndIconLock);
-
+
+ if (m_retainOrReleaseIconRequested)
+ performPendingRetainAndReleaseOperations();
+
for (unsigned i = 0; i < urls.size(); ++i) {
if (!m_retainedPageURLs.contains(urls[i])) {
PageURLRecord* record = m_pageURLToRecordMap.get(urls[i]);
@@ -1387,6 +1416,11 @@
// Then, if the thread should be quitting, quit now!
if (m_threadTerminationRequested)
break;
+
+ if (m_retainOrReleaseIconRequested) {
+ MutexLocker locker(m_urlAndIconLock);
+ performPendingRetainAndReleaseOperations();
+ }
bool didAnyWork = true;
while (didAnyWork) {
@@ -1471,6 +1505,25 @@
}
}
+void IconDatabase::performPendingRetainAndReleaseOperations()
+{
+ ASSERT(m_retainOrReleaseIconRequested);
+
+ // NOTE: The caller is assumed to hold m_urlAndIconLock.
+ ASSERT(!m_urlAndIconLock.tryLock());
+
+ MutexLocker vectorLocker(m_urlsToRetainOrReleaseLock);
+
+ for (HashCountedSet<String>::const_iterator it = m_urlsToRetain.begin(), end = m_urlsToRetain.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)
+ performReleaseIconForPageURL(it->first, it->second);
+
+ m_urlsToRetain.clear();
+ m_urlsToRelease.clear();
+ m_retainOrReleaseIconRequested = false;
+}
+
bool IconDatabase::readFromDatabase()
{
ASSERT_ICON_SYNC_THREAD();
Modified: trunk/Source/WebCore/loader/icon/IconDatabase.h (117500 => 117501)
--- trunk/Source/WebCore/loader/icon/IconDatabase.h 2012-05-17 21:46:02 UTC (rev 117500)
+++ trunk/Source/WebCore/loader/icon/IconDatabase.h 2012-05-17 21:49:43 UTC (rev 117501)
@@ -29,6 +29,7 @@
#include "IconDatabaseBase.h"
#include "Timer.h"
+#include <wtf/HashCountedSet.h>
#include <wtf/HashMap.h>
#include <wtf/HashSet.h>
#include <wtf/Noncopyable.h>
@@ -135,6 +136,11 @@
RefPtr<IconRecord> m_defaultIconRecord;
+ static void performScheduleOrDeferSyncTimerOnMainThread(void*);
+ void performScheduleOrDeferSyncTimer();
+
+ bool m_scheduleOrDeferSyncTimerRequested;
+
// *** Any Thread ***
public:
virtual bool isOpen() const;
@@ -159,6 +165,7 @@
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
@@ -177,6 +184,11 @@
HashSet<String> m_pageURLsInterestedInIcons;
HashSet<IconRecord*> m_iconsPendingReading;
+ Mutex m_urlsToRetainOrReleaseLock;
+ // Holding m_urlsToRetainOrReleaseLock is required when accessing any of the following data structures.
+ HashCountedSet<String> m_urlsToRetain;
+ HashCountedSet<String> m_urlsToRelease;
+
// *** Sync Thread Only ***
public:
// Should be used only on the sync thread and only by the Safari 2 Icons import procedure
@@ -202,6 +214,8 @@
void removeAllIconsOnThread();
void deleteAllPreparedStatements();
void* cleanupSyncThread();
+ void performRetainIconForPageURL(const String&, int retainCount);
+ void performReleaseIconForPageURL(const String&, int releaseCount);
// Record (on disk) whether or not Safari 2-style icons were imported (once per dataabse)
bool imported();
@@ -220,7 +234,9 @@
PassRefPtr<SharedBuffer> getImageDataForIconURLFromSQLDatabase(const String& iconURL);
void removeIconFromSQLDatabase(const String& iconURL);
void writeIconSnapshotToSQLDatabase(const IconSnapshot&);
-
+
+ void performPendingRetainAndReleaseOperations();
+
// Methods to dispatch client callbacks on the main thread
void dispatchDidImportIconURLForPageURLOnMainThread(const String&);
void dispatchDidImportIconDataForPageURLOnMainThread(const String&);
Modified: trunk/Source/WebCore/loader/icon/PageURLRecord.h (117500 => 117501)
--- trunk/Source/WebCore/loader/icon/PageURLRecord.h 2012-05-17 21:46:02 UTC (rev 117500)
+++ trunk/Source/WebCore/loader/icon/PageURLRecord.h 2012-05-17 21:49:43 UTC (rev 117501)
@@ -69,13 +69,19 @@
PageURLSnapshot snapshot(bool forDeletion = false) const;
// Returns false if the page wasn't retained beforehand, true if the retain count was already 1 or higher
- inline bool retain() { return m_retainCount++; }
+ bool retain(int count)
+ {
+ bool wasRetained = m_retainCount > 0;
+ m_retainCount += count;
+ return wasRetained;
+ }
// Returns true if the page is still retained after the call. False if the retain count just dropped to 0
- inline bool release()
+ bool release(int count)
{
- ASSERT(m_retainCount > 0);
- return --m_retainCount;
+ ASSERT(m_retainCount >= count);
+ m_retainCount -= count;
+ return m_retainCount > 0;
}
inline int retainCount() const { return m_retainCount; }
