Changeset 278778 in webkit

Timestamp:

Jun 11, 2021, 1:34:28 PM (5 years ago)

Author:

Chris Dumez

Message:

Regression(r276653) We're going to disk more often for local storage operations
​https://bugs.webkit.org/show_bug.cgi?id=226832

Reviewed by Darin Adler.

We're going to disk more often for local storage operations since r276653 because we no
longer keep items in memory. This results in a slightly increased power usage on one of
our benchmarks. As a first step to improve this, I am reintroducing a cache of the items
in memory, as long as the values are not too large (1Kb limit). We still go to disk to
look up values that are larger than 1Kb to avoid regressing memory usage.

• NetworkProcess/WebStorage/LocalStorageDatabase.cpp:

(WebKit::LocalStorageDatabase::openDatabase):
(WebKit::LocalStorageDatabase::items const):
(WebKit::LocalStorageDatabase::removeItem):
(WebKit::LocalStorageDatabase::item const):
(WebKit::LocalStorageDatabase::itemBypassingCache const):
(WebKit::LocalStorageDatabase::setItem):
(WebKit::LocalStorageDatabase::clear):
(WebKit::LocalStorageDatabase::close):
(WebKit::LocalStorageDatabase::databaseIsEmpty const):

• NetworkProcess/WebStorage/LocalStorageDatabase.h:

Location:

trunk/Source/WebKit

Files:

• ChangeLog (modified) (1 diff)
• NetworkProcess/WebStorage/LocalStorageDatabase.cpp (modified) (11 diffs)
• NetworkProcess/WebStorage/LocalStorageDatabase.h (modified) (2 diffs)

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2021-06-11  Chris Dumez  <cdumez@apple.com>
+
+        Regression(r276653) We're going to disk more often for local storage operations
+        https://bugs.webkit.org/show_bug.cgi?id=226832
+
+        Reviewed by Darin Adler.
+
+        We're going to disk more often for local storage operations since r276653 because we no
+        longer keep items in memory. This results in a slightly increased power usage on one of
+        our benchmarks. As a first step to improve this, I am reintroducing a cache of the items
+        in memory, as long as the values are not too large (1Kb limit). We still go to disk to
+        look up values that are larger than 1Kb to avoid regressing memory usage.
+
+        * NetworkProcess/WebStorage/LocalStorageDatabase.cpp:
+        (WebKit::LocalStorageDatabase::openDatabase):
+        (WebKit::LocalStorageDatabase::items const):
+        (WebKit::LocalStorageDatabase::removeItem):
+        (WebKit::LocalStorageDatabase::item const):
+        (WebKit::LocalStorageDatabase::itemBypassingCache const):
+        (WebKit::LocalStorageDatabase::setItem):
+        (WebKit::LocalStorageDatabase::clear):
+        (WebKit::LocalStorageDatabase::close):
+        (WebKit::LocalStorageDatabase::databaseIsEmpty const):
+        * NetworkProcess/WebStorage/LocalStorageDatabase.h:
+
 2021-06-11  Chris Dumez  <cdumez@apple.com>
 

trunk/Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabase.cpp

@@ -39 +39 @@
 using namespace WebCore;
 
-static const ASCIILiteral getItemsQueryString { "SELECT key, value FROM ItemTable"_s };
+constexpr auto getItemsQueryString { "SELECT key, value FROM ItemTable"_s };
+constexpr unsigned maximumSizeForValuesKeptInMemory { 1024 }; // 1 KB
 
 Ref<LocalStorageDatabase> LocalStorageDatabase::create(String&& databasePath, unsigned quotaInBytes)
@@ -62 +63 @@
 {
     ASSERT(!RunLoop::isMain());
-    if (!FileSystem::fileExists(m_databasePath) && shouldCreateDatabase == ShouldCreateDatabase::No)
-        return true;
+    if (!FileSystem::fileExists(m_databasePath)) {
+        if (shouldCreateDatabase == ShouldCreateDatabase::No)
+            return true;
+        m_items = HashMap<String, String> { };
+    }
 
     if (m_databasePath.isEmpty()) {
@@ -141 +145 @@
         return { };
 
+    HashMap<String, String> items;
+    if (m_items) {
+        items.reserveInitialCapacity(m_items->size());
+        for (auto& entry : *m_items) {
+            auto value = entry.value.isNull() ? itemBypassingCache(entry.key) : entry.value;
+            items.add(entry.key, WTFMove(value));
+        }
+        return items;
+    }
+
     auto query = scopedStatement(m_getItemsStatement, getItemsQueryString);
     if (!query) {
@@ -147 +161 @@
     }
 
-    HashMap<String, String> items;
+    m_items = HashMap<String, String> { };
     int result = query->step();
     while (result == SQLITE_ROW) {
         String key = query->columnText(0);
         String value = query->columnBlobAsString(1);
-        if (!key.isNull() && !value.isNull())
+        if (!key.isNull() && !value.isNull()) {
+            m_items->add(key, value.sizeInBytes() > maximumSizeForValuesKeptInMemory ? String() : value);
             items.add(WTFMove(key), WTFMove(value));
+        }
         result = query->step();
     }
@@ -185 +201 @@
         return;
     }
+
+    if (m_items)
+        m_items->remove(key);
 }
 
@@ -193 +212 @@
         return { };
 
+    if (m_items) {
+        // Use find() instead of get() since a null String is a valid value here.
+        auto it = m_items->find(key);
+        if (it == m_items->end())
+            return { };
+        if (!it->value.isNull())
+            return it->value;
+        // The value is too large and needs to be fetched from the database.
+    }
+    return itemBypassingCache(key);
+}
+
+String LocalStorageDatabase::itemBypassingCache(const String& key) const
+{
     auto query = scopedStatement(m_getItemStatement, "SELECT value FROM ItemTable WHERE key=?"_s);
     if (!query) {
@@ -232 +265 @@
         if (result == SQLITE_FULL)
             quotaException = true;
-    }
+        return;
+    }
+
+    if (m_items)
+        m_items->set(key, value.sizeInBytes() > maximumSizeForValuesKeptInMemory ? String() : value);
 }
 
@@ -239 +276 @@
     ASSERT(!RunLoop::isMain());
     if (!m_database.isOpen())
+        return false;
+
+    if (m_items && m_items->isEmpty())
         return false;
 
@@ -251 +291 @@
         LOG_ERROR("Failed to clear all items in the local storage database - %i", result);
         return false;
+    }
+
+    if (m_items) {
+        m_items->clear();
+        return true;
     }
 
@@ -270 +315 @@
     m_getItemsStatement = nullptr;
     m_deleteItemStatement = nullptr;
+    m_items = std::nullopt;
 
     if (m_database.isOpen())
@@ -283 +329 @@
     if (!m_database.isOpen())
         return false;
+
+    if (m_items)
+        return m_items->isEmpty();
 
     auto query = m_database.prepareStatement("SELECT COUNT(*) FROM ItemTable"_s);

trunk/Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabase.h

@@ -63 +63 @@
     bool databaseIsEmpty() const;
 
+    String itemBypassingCache(const String& key) const;
+
     WebCore::SQLiteStatementAutoResetScope scopedStatement(std::unique_ptr<WebCore::SQLiteStatement>&, ASCIILiteral query) const;
 
@@ -69 +71 @@
     const unsigned m_quotaInBytes { 0 };
     bool m_isClosed { false };
+
+    // Cached version of the items in memory.
+    // If the value is too large to keep in memory, we store a null String.
+    mutable std::optional<HashMap<String, String>> m_items;
 
     mutable std::unique_ptr<WebCore::SQLiteStatement> m_clearStatement;