<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[55523] trunk/WebCore</title>
</head>
<body>

<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt;  }
#msg dl a { font-weight: bold}
#msg dl a:link    { color:#fc3; }
#msg dl a:active  { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff  {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/55523">55523</a></dd>
<dt>Author</dt> <dd>jorlow@chromium.org</dd>
<dt>Date</dt> <dd>2010-03-04 06:24:35 -0800 (Thu, 04 Mar 2010)</dd>
</dl>

<h3>Log Message</h3>
<pre>Trottle sync requests sent to the LocalStorage background thread
https://bugs.webkit.org/show_bug.cgi?id=34943

Reviewed by Darin Fisher.

Currently, once a second LocalStorage takes all keys/values which have
been changed and sends them to a background thread to sync.  The problem
is that this background thread can get overwhelmed and stop being
responsive.  This means that if any other page tries to start using
LocalStorage (and thus initiates the initial import) that'll block on
all the previous syncs completing.

To mitigate this, I'm adding code so that we never schedule another
sync task when another is still running.  In order to keep the sync
tasks from growing exponentially when they do take longer than the
storage sync interval, I've also added a basic rate limiter.  No effort
is made to ensure fairness/ordering of what gets synced nor is there
any way for this rate to be changed because most normal uses of
LocalStorage really shouldn't be hitting these types of limits anyway.

The only behavioral change that's observible in JavaScript is time based
and thus it's not practical to make new tests that aren't racy.  The
existing layout tests cover LocalStorage pretty well, though.

* storage/StorageAreaSync.cpp:
(WebCore::StorageAreaSync::StorageAreaSync):
(WebCore::StorageAreaSync::scheduleFinalSync):
(WebCore::StorageAreaSync::syncTimerFired):
(WebCore::StorageAreaSync::performSync):
* storage/StorageAreaSync.h:</pre>

<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkWebCoreChangeLog">trunk/WebCore/ChangeLog</a></li>
<li><a href="#trunkWebCorestorageStorageAreaSynccpp">trunk/WebCore/storage/StorageAreaSync.cpp</a></li>
<li><a href="#trunkWebCorestorageStorageAreaSynch">trunk/WebCore/storage/StorageAreaSync.h</a></li>
</ul>

</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkWebCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/WebCore/ChangeLog (55522 => 55523)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/WebCore/ChangeLog        2010-03-04 14:00:40 UTC (rev 55522)
+++ trunk/WebCore/ChangeLog        2010-03-04 14:24:35 UTC (rev 55523)
</span><span class="lines">@@ -1,3 +1,36 @@
</span><ins>+2010-03-03  Jeremy Orlow  &lt;jorlow@chromium.org&gt;
+
+        Reviewed by Darin Fisher.
+
+        Trottle sync requests sent to the LocalStorage background thread
+        https://bugs.webkit.org/show_bug.cgi?id=34943
+
+        Currently, once a second LocalStorage takes all keys/values which have
+        been changed and sends them to a background thread to sync.  The problem
+        is that this background thread can get overwhelmed and stop being
+        responsive.  This means that if any other page tries to start using
+        LocalStorage (and thus initiates the initial import) that'll block on
+        all the previous syncs completing.
+
+        To mitigate this, I'm adding code so that we never schedule another
+        sync task when another is still running.  In order to keep the sync
+        tasks from growing exponentially when they do take longer than the
+        storage sync interval, I've also added a basic rate limiter.  No effort
+        is made to ensure fairness/ordering of what gets synced nor is there
+        any way for this rate to be changed because most normal uses of
+        LocalStorage really shouldn't be hitting these types of limits anyway.
+
+        The only behavioral change that's observible in JavaScript is time based
+        and thus it's not practical to make new tests that aren't racy.  The
+        existing layout tests cover LocalStorage pretty well, though.
+
+        * storage/StorageAreaSync.cpp:
+        (WebCore::StorageAreaSync::StorageAreaSync):
+        (WebCore::StorageAreaSync::scheduleFinalSync):
+        (WebCore::StorageAreaSync::syncTimerFired):
+        (WebCore::StorageAreaSync::performSync):
+        * storage/StorageAreaSync.h:
+
</ins><span class="cx"> 2010-03-04  Andrey Kosyakov  &lt;caseq@chromium.org&gt;
</span><span class="cx"> 
</span><span class="cx">         Reviewed by Pavel Feldman.
</span></span></pre></div>
<a id="trunkWebCorestorageStorageAreaSynccpp"></a>
<div class="modfile"><h4>Modified: trunk/WebCore/storage/StorageAreaSync.cpp (55522 => 55523)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/WebCore/storage/StorageAreaSync.cpp        2010-03-04 14:00:40 UTC (rev 55522)
+++ trunk/WebCore/storage/StorageAreaSync.cpp        2010-03-04 14:24:35 UTC (rev 55523)
</span><span class="lines">@@ -43,6 +43,10 @@
</span><span class="cx"> // Instead, queue up a batch of items to sync and actually do the sync at the following interval.
</span><span class="cx"> static const double StorageSyncInterval = 1.0;
</span><span class="cx"> 
</span><ins>+// A sane limit on how many items we'll schedule to sync all at once.  This makes it
+// much harder to starve the rest of LocalStorage and the OS's IO subsystem in general.
+static const int MaxiumItemsToSync = 100;
+
</ins><span class="cx"> PassRefPtr&lt;StorageAreaSync&gt; StorageAreaSync::create(PassRefPtr&lt;StorageSyncManager&gt; storageSyncManager, PassRefPtr&lt;StorageAreaImpl&gt; storageArea, String databaseIdentifier)
</span><span class="cx"> {
</span><span class="cx">     return adoptRef(new StorageAreaSync(storageSyncManager, storageArea, databaseIdentifier));
</span><span class="lines">@@ -57,6 +61,7 @@
</span><span class="cx">     , m_databaseIdentifier(databaseIdentifier.crossThreadString())
</span><span class="cx">     , m_clearItemsWhileSyncing(false)
</span><span class="cx">     , m_syncScheduled(false)
</span><ins>+    , m_syncInProgress(false)
</ins><span class="cx">     , m_importComplete(false)
</span><span class="cx"> {
</span><span class="cx">     ASSERT(isMainThread());
</span><span class="lines">@@ -92,8 +97,8 @@
</span><span class="cx">     }
</span><span class="cx">     // FIXME: This is synchronous.  We should do it on the background process, but
</span><span class="cx">     // we should do it safely.
</span><ins>+    m_finalSyncScheduled = true;
</ins><span class="cx">     syncTimerFired(&amp;m_syncTimer);
</span><del>-    m_finalSyncScheduled = true;
</del><span class="cx"> }
</span><span class="cx"> 
</span><span class="cx"> void StorageAreaSync::scheduleItemForSync(const String&amp; key, const String&amp; value)
</span><span class="lines">@@ -131,21 +136,44 @@
</span><span class="cx"> {
</span><span class="cx">     ASSERT(isMainThread());
</span><span class="cx"> 
</span><del>-    HashMap&lt;String, String&gt;::iterator it = m_changedItems.begin();
-    HashMap&lt;String, String&gt;::iterator end = m_changedItems.end();
-
</del><ins>+    bool partialSync = false;
</ins><span class="cx">     {
</span><span class="cx">         MutexLocker locker(m_syncLock);
</span><span class="cx"> 
</span><ins>+        // Do not schedule another sync if we're still trying to complete the
+        // previous one.  But, if we're shutting down, schedule it anyway.
+        if (m_syncInProgress &amp;&amp; !m_finalSyncScheduled) {
+            ASSERT(!m_syncTimer.isActive());
+            m_syncTimer.startOneShot(StorageSyncInterval);
+            return;
+        }
+
</ins><span class="cx">         if (m_itemsCleared) {
</span><span class="cx">             m_itemsPendingSync.clear();
</span><span class="cx">             m_clearItemsWhileSyncing = true;
</span><span class="cx">             m_itemsCleared = false;
</span><span class="cx">         }
</span><span class="cx"> 
</span><del>-        for (; it != end; ++it)
-            m_itemsPendingSync.set(it-&gt;first.crossThreadString(), it-&gt;second.crossThreadString());
</del><ins>+        HashMap&lt;String, String&gt;::iterator changed_it = m_changedItems.begin();
+        HashMap&lt;String, String&gt;::iterator changed_end = m_changedItems.end();
+        for (int count = 0; changed_it != changed_end; ++count, ++changed_it) {
+            if (count &gt;= MaxiumItemsToSync &amp;&amp; !m_finalSyncScheduled) {
+                partialSync = true;
+                break;
+            }
+            m_itemsPendingSync.set(changed_it-&gt;first.crossThreadString(), changed_it-&gt;second.crossThreadString());
+        }
</ins><span class="cx"> 
</span><ins>+        if (partialSync) {
+            // We can't do the fast path of simply clearing all items, so we'll need to manually
+            // remove them one by one.  Done under lock since m_itemsPendingSync is modified by
+            // the background thread.
+            HashMap&lt;String, String&gt;::iterator pending_it = m_itemsPendingSync.begin();
+            HashMap&lt;String, String&gt;::iterator pending_end = m_itemsPendingSync.end();
+            for (; pending_it != pending_end; ++pending_it)
+                m_changedItems.remove(pending_it-&gt;first);
+        }
+
</ins><span class="cx">         if (!m_syncScheduled) {
</span><span class="cx">             m_syncScheduled = true;
</span><span class="cx"> 
</span><span class="lines">@@ -157,11 +185,17 @@
</span><span class="cx">         }
</span><span class="cx">     }
</span><span class="cx"> 
</span><del>-    // The following is balanced by the calls to disableSuddenTermination in the
-    // scheduleItemForSync, scheduleClear, and scheduleFinalSync functions.
-    enableSuddenTermination();
</del><ins>+    if (partialSync) {
+        // If we didn't finish syncing, then we need to finish the job later.
+        ASSERT(!m_syncTimer.isActive());
+        m_syncTimer.startOneShot(StorageSyncInterval);
+    } else {
+        // The following is balanced by the calls to disableSuddenTermination in the
+        // scheduleItemForSync, scheduleClear, and scheduleFinalSync functions.
+        enableSuddenTermination();
</ins><span class="cx"> 
</span><del>-    m_changedItems.clear();
</del><ins>+        m_changedItems.clear();
+    }
</ins><span class="cx"> }
</span><span class="cx"> 
</span><span class="cx"> void StorageAreaSync::performImport()
</span><span class="lines">@@ -319,10 +353,16 @@
</span><span class="cx"> 
</span><span class="cx">         m_clearItemsWhileSyncing = false;
</span><span class="cx">         m_syncScheduled = false;
</span><ins>+        m_syncInProgress = true;
</ins><span class="cx">     }
</span><span class="cx"> 
</span><span class="cx">     sync(clearItems, items);
</span><span class="cx"> 
</span><ins>+    {
+        MutexLocker locker(m_syncLock);
+        m_syncInProgress = false;
+    }
+
</ins><span class="cx">     // The following is balanced by the call to disableSuddenTermination in the
</span><span class="cx">     // syncTimerFired function.
</span><span class="cx">     enableSuddenTermination();
</span></span></pre></div>
<a id="trunkWebCorestorageStorageAreaSynch"></a>
<div class="modfile"><h4>Modified: trunk/WebCore/storage/StorageAreaSync.h (55522 => 55523)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/WebCore/storage/StorageAreaSync.h        2010-03-04 14:00:40 UTC (rev 55522)
+++ trunk/WebCore/storage/StorageAreaSync.h        2010-03-04 14:24:35 UTC (rev 55523)
</span><span class="lines">@@ -84,6 +84,7 @@
</span><span class="cx">         HashMap&lt;String, String&gt; m_itemsPendingSync;
</span><span class="cx">         bool m_clearItemsWhileSyncing;
</span><span class="cx">         bool m_syncScheduled;
</span><ins>+        bool m_syncInProgress;
</ins><span class="cx"> 
</span><span class="cx">         mutable Mutex m_importLock;
</span><span class="cx">         mutable ThreadCondition m_importCondition;
</span></span></pre>
</div>
</div>

</body>
</html>