[SessionStorage] Optimizing opening & forking of empty storage areas.

There are still a large number of renderer hangs from GetAll. Since
the other possible hang call sites have been marked [Sync], this means
that the loading of the storage area map is the final obvious area
where this hang could be occurring.

mek@ and I found a number of possible optimization spots where a map is
known to be empty during the GetAll (or Fork) calls, and in these cases
the code can avoid hitting disk. Hopefully this captures many common
cases and can help the code hit disk less.

https://docs.google.com/document/d/1Pw8W74-7NCHpxrtglVHqFZpLV_p2JFwdMNqo03kx2l8/edit
has the discussion notes.

R=mek@chromium.org

Bug: 897581
Change-Id: I92de5848ebf17e2ab4fd5cd85610922858e629a8
Reviewed-on: https://chromium-review.googlesource.com/c/1488178
Commit-Queue: Daniel Murphy <dmurph@chromium.org>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#635790}

content/browser/dom_storage/session_storage_area_impl.cc

@@ -145,7 +145,7 @@ void SessionStorageAreaImpl::CreateNewMap(
       // The code optimizes the 'delete all' for shared maps by just creating
       // a new map instead of forking. However, we still need the observers to
       // be correctly called. To do that, we manually call them here.
-      shared_data_map_ = SessionStorageDataMap::Create(
+      shared_data_map_ = SessionStorageDataMap::CreateEmpty(
           shared_data_map_->listener(),
           register_new_map_callback_.Run(namespace_entry_, origin_),
           shared_data_map_->storage_area()->database());

content/browser/dom_storage/session_storage_area_impl_unittest.cc

@@ -127,7 +127,7 @@ TEST_F(SessionStorageAreaImplTest, BasicUsage) {
 
   auto ss_leveldb_impl = std::make_unique<SessionStorageAreaImpl>(
       metadata_.GetOrCreateNamespaceEntry(test_namespace_id1_), test_origin1_,
-      SessionStorageDataMap::Create(
+      SessionStorageDataMap::CreateFromDisk(
           &listener_,
           metadata_.GetOrCreateNamespaceEntry(test_namespace_id1_)
               ->second[test_origin1_],
@@ -149,6 +149,32 @@ TEST_F(SessionStorageAreaImplTest, BasicUsage) {
       .Times(1);
 }
 
+TEST_F(SessionStorageAreaImplTest, ExplicitlyEmptyMap) {
+  EXPECT_CALL(listener_,
+              OnDataMapCreation(StdStringToUint8Vector("0"), testing::_))
+      .Times(1);
+
+  auto ss_leveldb_impl = std::make_unique<SessionStorageAreaImpl>(
+      metadata_.GetOrCreateNamespaceEntry(test_namespace_id1_), test_origin1_,
+      SessionStorageDataMap::CreateEmpty(
+          &listener_,
+          metadata_.GetOrCreateNamespaceEntry(test_namespace_id1_)
+              ->second[test_origin1_],
+          leveldb_database_.get()),
+      GetRegisterNewAreaMapCallback());
+
+  blink::mojom::StorageAreaAssociatedPtr ss_leveldb;
+  ss_leveldb_impl->Bind(
+      mojo::MakeRequestAssociatedWithDedicatedPipe(&ss_leveldb));
+
+  std::vector<blink::mojom::KeyValuePtr> data;
+  EXPECT_TRUE(test::GetAllSync(ss_leveldb.get(), &data));
+  ASSERT_EQ(0ul, data.size());
+
+  EXPECT_CALL(listener_, OnDataMapDestruction(StdStringToUint8Vector("0")))
+      .Times(1);
+}
+
 TEST_F(SessionStorageAreaImplTest, DoubleBind) {
   EXPECT_CALL(listener_,
               OnDataMapCreation(StdStringToUint8Vector("0"), testing::_))
@@ -156,7 +182,7 @@ TEST_F(SessionStorageAreaImplTest, DoubleBind) {
 
   auto ss_leveldb_impl = std::make_unique<SessionStorageAreaImpl>(
       metadata_.GetOrCreateNamespaceEntry(test_namespace_id1_), test_origin1_,
-      SessionStorageDataMap::Create(
+      SessionStorageDataMap::CreateFromDisk(
           &listener_,
           metadata_.GetOrCreateNamespaceEntry(test_namespace_id1_)
               ->second[test_origin1_],
@@ -190,7 +216,7 @@ TEST_F(SessionStorageAreaImplTest, Cloning) {
 
   auto ss_leveldb_impl1 = std::make_unique<SessionStorageAreaImpl>(
       metadata_.GetOrCreateNamespaceEntry(test_namespace_id1_), test_origin1_,
-      SessionStorageDataMap::Create(
+      SessionStorageDataMap::CreateFromDisk(
           &listener_,
           metadata_.GetOrCreateNamespaceEntry(test_namespace_id1_)
               ->second[test_origin1_],
@@ -265,7 +291,7 @@ TEST_F(SessionStorageAreaImplTest, NotifyAllDeleted) {
 
   auto ss_leveldb_impl1 = std::make_unique<SessionStorageAreaImpl>(
       metadata_.GetOrCreateNamespaceEntry(test_namespace_id1_), test_origin1_,
-      SessionStorageDataMap::Create(
+      SessionStorageDataMap::CreateFromDisk(
           &listener_,
           metadata_.GetOrCreateNamespaceEntry(test_namespace_id1_)
               ->second[test_origin1_],
@@ -301,7 +327,7 @@ TEST_F(SessionStorageAreaImplTest, DeleteAllOnShared) {
 
   auto ss_leveldb_impl1 = std::make_unique<SessionStorageAreaImpl>(
       metadata_.GetOrCreateNamespaceEntry(test_namespace_id1_), test_origin1_,
-      SessionStorageDataMap::Create(
+      SessionStorageDataMap::CreateFromDisk(
           &listener_,
           metadata_.GetOrCreateNamespaceEntry(test_namespace_id1_)
               ->second[test_origin1_],
@@ -367,7 +393,7 @@ TEST_F(SessionStorageAreaImplTest, DeleteAllWithoutBinding) {
 
   auto ss_leveldb_impl1 = std::make_unique<SessionStorageAreaImpl>(
       metadata_.GetOrCreateNamespaceEntry(test_namespace_id1_), test_origin1_,
-      SessionStorageDataMap::Create(
+      SessionStorageDataMap::CreateFromDisk(
           &listener_,
           metadata_.GetOrCreateNamespaceEntry(test_namespace_id1_)
               ->second[test_origin1_],

content/browser/dom_storage/session_storage_data_map.cc

@@ -12,12 +12,21 @@
 namespace content {
 
 // static
-scoped_refptr<SessionStorageDataMap> SessionStorageDataMap::Create(
+scoped_refptr<SessionStorageDataMap> SessionStorageDataMap::CreateFromDisk(
+    Listener* listener,
+    scoped_refptr<SessionStorageMetadata::MapData> map_data,
+    leveldb::mojom::LevelDBDatabase* database) {
+  return base::WrapRefCounted(new SessionStorageDataMap(
+      listener, std::move(map_data), database, false));
+}
+
+// static
+scoped_refptr<SessionStorageDataMap> SessionStorageDataMap::CreateEmpty(
     Listener* listener,
     scoped_refptr<SessionStorageMetadata::MapData> map_data,
     leveldb::mojom::LevelDBDatabase* database) {
   return base::WrapRefCounted(
-      new SessionStorageDataMap(listener, std::move(map_data), database));
+      new SessionStorageDataMap(listener, std::move(map_data), database, true));
 }
 
 // static
@@ -41,7 +50,8 @@ void SessionStorageDataMap::DidCommit(leveldb::mojom::DatabaseError error) {
 SessionStorageDataMap::SessionStorageDataMap(
     Listener* listener,
     scoped_refptr<SessionStorageMetadata::MapData> map_data,
-    leveldb::mojom::LevelDBDatabase* database)
+    leveldb::mojom::LevelDBDatabase* database,
+    bool is_empty)
     : listener_(listener),
       map_data_(std::move(map_data)),
       storage_area_impl_(
@@ -50,6 +60,8 @@ SessionStorageDataMap::SessionStorageDataMap(
                                             this,
                                             GetOptions())),
       storage_area_ptr_(storage_area_impl_.get()) {
+  if (is_empty)
+    storage_area_impl_->InitializeAsEmpty();
   DCHECK(listener_);
   DCHECK(map_data_);
   listener_->OnDataMapCreation(map_data_->MapNumberAsBytes(), this);

content/browser/dom_storage/session_storage_data_map.h

@@ -41,7 +41,12 @@ class CONTENT_EXPORT SessionStorageDataMap final
     virtual void OnCommitResult(leveldb::mojom::DatabaseError error) = 0;
   };
 
-  static scoped_refptr<SessionStorageDataMap> Create(
+  static scoped_refptr<SessionStorageDataMap> CreateFromDisk(
+      Listener* listener,
+      scoped_refptr<SessionStorageMetadata::MapData> map_data,
+      leveldb::mojom::LevelDBDatabase* database);
+
+  static scoped_refptr<SessionStorageDataMap> CreateEmpty(
       Listener* listener,
       scoped_refptr<SessionStorageMetadata::MapData> map_data,
       leveldb::mojom::LevelDBDatabase* database);
@@ -78,7 +83,8 @@ class CONTENT_EXPORT SessionStorageDataMap final
   SessionStorageDataMap(
       Listener* listener,
       scoped_refptr<SessionStorageMetadata::MapData> map_entry,
-      leveldb::mojom::LevelDBDatabase* database);
+      leveldb::mojom::LevelDBDatabase* database,
+      bool is_empty);
   SessionStorageDataMap(
       Listener* listener,
       scoped_refptr<SessionStorageMetadata::MapData> map_entry,

content/browser/dom_storage/session_storage_data_map_unittest.cc

@@ -104,10 +104,12 @@ TEST_F(SessionStorageDataMapTest, BasicEmptyCreation) {
               OnDataMapCreation(StdStringToUint8Vector("1"), testing::_))
       .Times(1);
 
-  scoped_refptr<SessionStorageDataMap> map = SessionStorageDataMap::Create(
-      &listener_,
-      base::MakeRefCounted<SessionStorageMetadata::MapData>(1, test_origin_),
-      &database_);
+  scoped_refptr<SessionStorageDataMap> map =
+      SessionStorageDataMap::CreateFromDisk(
+          &listener_,
+          base::MakeRefCounted<SessionStorageMetadata::MapData>(1,
+                                                                test_origin_),
+          &database_);
 
   bool success;
   std::vector<blink::mojom::KeyValuePtr> data;
@@ -131,16 +133,48 @@ TEST_F(SessionStorageDataMapTest, BasicEmptyCreation) {
   EXPECT_EQ(2u, mock_data_.size());
 }
 
-TEST_F(SessionStorageDataMapTest, Clone) {
+TEST_F(SessionStorageDataMapTest, ExplicitlyEmpty) {
   EXPECT_CALL(listener_,
               OnDataMapCreation(StdStringToUint8Vector("1"), testing::_))
       .Times(1);
 
-  scoped_refptr<SessionStorageDataMap> map1 = SessionStorageDataMap::Create(
+  scoped_refptr<SessionStorageDataMap> map = SessionStorageDataMap::CreateEmpty(
       &listener_,
       base::MakeRefCounted<SessionStorageMetadata::MapData>(1, test_origin_),
       &database_);
 
+  bool success;
+  std::vector<blink::mojom::KeyValuePtr> data;
+  bool done = false;
+  base::RunLoop loop;
+  map->storage_area()->GetAll(
+      GetAllCallback::CreateAndBind(&done, loop.QuitClosure()),
+      MakeGetAllCallback(&success, &data));
+  loop.Run();
+
+  EXPECT_TRUE(done);
+  ASSERT_EQ(0u, data.size());
+
+  EXPECT_CALL(listener_, OnDataMapDestruction(StdStringToUint8Vector("1")))
+      .Times(1);
+
+  // Test data is not cleared on deletion.
+  map = nullptr;
+  EXPECT_EQ(2u, mock_data_.size());
+}
+
+TEST_F(SessionStorageDataMapTest, Clone) {
+  EXPECT_CALL(listener_,
+              OnDataMapCreation(StdStringToUint8Vector("1"), testing::_))
+      .Times(1);
+
+  scoped_refptr<SessionStorageDataMap> map1 =
+      SessionStorageDataMap::CreateFromDisk(
+          &listener_,
+          base::MakeRefCounted<SessionStorageMetadata::MapData>(1,
+                                                                test_origin_),
+          &database_);
+
   EXPECT_CALL(listener_,
               OnDataMapCreation(StdStringToUint8Vector("2"), testing::_))
       .Times(1);

content/browser/dom_storage/session_storage_namespace_impl_mojo.cc

@@ -52,8 +52,8 @@ void SessionStorageNamespaceImplMojo::PopulateFromMetadata(
         delegate_->MaybeGetExistingDataMapForId(
             pair.second->MapNumberAsBytes());
     if (!data_map) {
-      data_map = SessionStorageDataMap::Create(data_map_listener_, pair.second,
-                                               database_);
+      data_map = SessionStorageDataMap::CreateFromDisk(data_map_listener_,
+                                                       pair.second, database_);
     }
     origin_areas_[pair.first] = std::make_unique<SessionStorageAreaImpl>(
         namespace_entry_, pair.first, std::move(data_map),
@@ -160,16 +160,19 @@ void SessionStorageNamespaceImplMojo::OpenArea(
     scoped_refptr<SessionStorageDataMap> data_map;
     auto map_data_it = namespace_entry_->second.find(origin);
     if (map_data_it != namespace_entry_->second.end()) {
+      // The map exists already, either on disk or being used by another
+      // namespace.
       scoped_refptr<SessionStorageMetadata::MapData> map_data =
           map_data_it->second;
       data_map =
           delegate_->MaybeGetExistingDataMapForId(map_data->MapNumberAsBytes());
       if (!data_map) {
-        data_map = SessionStorageDataMap::Create(data_map_listener_, map_data,
-                                                 database_);
+        data_map = SessionStorageDataMap::CreateFromDisk(data_map_listener_,
+                                                         map_data, database_);
       }
     } else {
-      data_map = SessionStorageDataMap::Create(
+      // The map doesn't exist yet.
+      data_map = SessionStorageDataMap::CreateEmpty(
           data_map_listener_,
           register_new_map_callback_.Run(namespace_entry_, origin), database_);
     }

content/browser/dom_storage/storage_area_impl.cc

@@ -94,6 +94,13 @@ StorageAreaImpl::~StorageAreaImpl() {
     CommitChanges();
 }
 
+void StorageAreaImpl::InitializeAsEmpty() {
+  DCHECK_EQ(map_state_, MapState::UNLOADED);
+  map_state_ = MapState::LOADING_FROM_DATABASE;
+  OnMapLoaded(leveldb::mojom::DatabaseError::OK,
+              std::vector<leveldb::mojom::KeyValuePtr>());
+}
+
 void StorageAreaImpl::Bind(blink::mojom::StorageAreaRequest request) {
   bindings_.AddBinding(this, std::move(request));
   // If the number of bindings is more than 1, then the |client_old_value| sent
@@ -122,7 +129,11 @@ std::unique_ptr<StorageAreaImpl> StorageAreaImpl::ForkToNewPrefix(
     const Options& options) {
   auto forked_area = std::make_unique<StorageAreaImpl>(
       database_, std::move(new_prefix), delegate, options);
-
+  // If the source map is empty, don't bother hitting disk.
+  if (IsMapLoadedAndEmpty()) {
+    forked_area->InitializeAsEmpty();
+    return forked_area;
+  }
   forked_area->map_state_ = MapState::LOADING_FROM_FORK;
 
   if (IsMapLoaded()) {
@@ -487,6 +498,17 @@ void StorageAreaImpl::Get(const std::vector<uint8_t>& key,
 void StorageAreaImpl::GetAll(
     blink::mojom::StorageAreaGetAllCallbackAssociatedPtrInfo complete_callback,
     GetAllCallback callback) {
+  // If the map is keys-only and empty, then no loading is necessary.
+  if (IsMapLoadedAndEmpty()) {
+    std::move(callback).Run(true, std::vector<blink::mojom::KeyValuePtr>());
+    if (complete_callback.is_valid()) {
+      blink::mojom::StorageAreaGetAllCallbackAssociatedPtr complete_ptr;
+      complete_ptr.Bind(std::move(complete_callback));
+      complete_ptr->Complete(true);
+    }
+    return;
+  }
+
   // The map must always be loaded for the KEYS_ONLY_WHEN_POSSIBLE mode.
   if (map_state_ != MapState::LOADED_KEYS_AND_VALUES) {
     LoadMap(base::BindOnce(&StorageAreaImpl::GetAll,

content/browser/dom_storage/storage_area_impl.h

@@ -100,6 +100,15 @@ class CONTENT_EXPORT StorageAreaImpl : public blink::mojom::StorageArea {
 
   ~StorageAreaImpl() override;
 
+  // Initializes the storage area as loaded & empty. This can only be called
+  // immediately after construction, and before any other methods are called
+  // that would load data from the database.
+  // This avoids hitting disk to load a map that the implementer already knows
+  // must be empty. Do not use this option unless you are absolutely certain
+  // that there must be no data for the |prefix|, as the data will not be loaded
+  // to check.
+  void InitializeAsEmpty();
+
   void Bind(blink::mojom::StorageAreaRequest request);
 
   // Forks, or copies, all data in this prefix to another prefix.

content/browser/dom_storage/storage_area_impl_unittest.cc

@@ -264,6 +264,7 @@ class StorageAreaImplTest : public testing::Test,
   const std::vector<uint8_t> test_key2_bytes_ = ToBytes(test_key2_);
   const std::vector<uint8_t> test_value1_bytes_ = ToBytes(test_value1_);
   const std::vector<uint8_t> test_value2_bytes_ = ToBytes(test_value2_);
+  leveldb::mojom::LevelDBDatabasePtr level_db_database_ptr_;
 
  private:
   // LevelDBObserver:
@@ -300,7 +301,6 @@ class StorageAreaImplTest : public testing::Test,
   TestBrowserThreadBundle thread_bundle_;
   std::map<std::vector<uint8_t>, std::vector<uint8_t>> mock_data_;
   FakeLevelDBDatabase db_;
-  leveldb::mojom::LevelDBDatabasePtr level_db_database_ptr_;
   MockDelegate delegate_;
   std::unique_ptr<StorageAreaImpl> storage_area_;
   blink::mojom::StorageAreaPtr storage_area_ptr_;
@@ -1300,4 +1300,46 @@ TEST_F(StorageAreaImplTest, PrefixForkingPsuedoFuzzer) {
   }
 }
 
+TEST_P(StorageAreaImplParamTest, EmptyMapIgnoresDisk) {
+  const std::string kValue = "foo";
+  const std::vector<uint8_t> kValueVec = ToBytes(kValue);
+
+  // Set fake data to ensure that our shortcut doesn't read it.
+  set_mock_data(test_copy_prefix1_ + test_key1_, kValue);
+
+  // Create an empty map that will have no data in it.
+  StorageAreaImpl::Options options =
+      GetDefaultTestingOptions(CacheMode::KEYS_ONLY_WHEN_POSSIBLE);
+  auto empty_storage_area = std::make_unique<StorageAreaImpl>(
+      level_db_database_ptr_.get(), test_copy_prefix1_, delegate(), options);
+  empty_storage_area->InitializeAsEmpty();
+
+  // Check the forked state, which should be empty.
+  EXPECT_EQ("", GetSyncStrUsingGetAll(empty_storage_area.get(), test_key1_));
+}
+
+TEST_P(StorageAreaImplParamTest, ForkFromEmptyMap) {
+  const std::string kValue = "foo";
+  const std::vector<uint8_t> kValueVec = ToBytes(kValue);
+
+  // Set fake data to ensure that our shortcut doesn't read it.
+  set_mock_data(test_copy_prefix1_ + test_key1_, kValue);
+
+  // Create an empty map that will have no data in it.
+  StorageAreaImpl::Options options =
+      GetDefaultTestingOptions(CacheMode::KEYS_ONLY_WHEN_POSSIBLE);
+  auto empty_storage_area = std::make_unique<StorageAreaImpl>(
+      level_db_database_ptr_.get(), test_copy_prefix1_, delegate(), options);
+  empty_storage_area->InitializeAsEmpty();
+
+  // Execute the fork, which should shortcut disk and just be empty.
+  MockDelegate fork1_delegate;
+  std::unique_ptr<StorageAreaImpl> fork =
+      empty_storage_area->ForkToNewPrefix(test_copy_prefix1_, &fork1_delegate,
+                                          GetDefaultTestingOptions(GetParam()));
+
+  // Check the forked state, which should be empty.
+  EXPECT_EQ("", GetSyncStrUsingGetAll(fork.get(), test_key1_));
+}
+
 }  // namespace content