DomStorageArea: Allow multiple simultaneous in flight commit batches.

webkit/dom_storage/dom_storage_area.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "webkit/dom_storage/dom_storage_area.h"
 
 #include "base/bind.h"
 #include "base/file_util.h"
 #include "base/location.h"
 #include "base/logging.h"
@@ skipping 42 matching lines @@
 }
 
 DomStorageArea::DomStorageArea(
     int64 namespace_id, const GURL& origin,
     const FilePath& directory, DomStorageTaskRunner* task_runner)
     : namespace_id_(namespace_id), origin_(origin),
       directory_(directory),
       task_runner_(task_runner),
       map_(new DomStorageMap(kPerAreaQuota)),
       is_initial_import_done_(true),
-      is_shutdown_(false) {
+      is_shutdown_(false),
+      commit_batches_in_flight_(0) {
   if (namespace_id == kLocalStorageNamespaceId && !directory.empty()) {
     FilePath path = directory.Append(DatabaseFileNameFromOrigin(origin_));
     backing_.reset(new DomStorageDatabase(path));
     is_initial_import_done_ = false;
   }
 }
 
 DomStorageArea::~DomStorageArea() {
 }
 
@@ skipping 80 matching lines @@
 
   DomStorageArea* copy = new DomStorageArea(destination_namespace_id, origin_,
                                             FilePath(), task_runner_);
   copy->map_ = map_;
   copy->is_shutdown_ = is_shutdown_;
   return copy;
 }
 
 bool DomStorageArea::HasUncommittedChanges() const {
   DCHECK(!is_shutdown_);
-  return commit_batch_.get() || in_flight_commit_batch_.get();
+  return commit_batch_.get() || commit_batches_in_flight_;
 }
 
 void DomStorageArea::DeleteOrigin() {
   DCHECK(!is_shutdown_);
   if (HasUncommittedChanges()) {
     // TODO(michaeln): This logically deletes the data immediately,
     // and in a matter of a second, deletes the rows from the backing
     // database file, but the file itself will linger until shutdown
     // or purge time. Ideally, this should delete the file more
     // quickly.
@@ skipping 51 matching lines @@
   backing_->ReadAllValues(&initial_values);
   map_->SwapValues(&initial_values);
   is_initial_import_done_ = true;
 }
 
 DomStorageArea::CommitBatch* DomStorageArea::CreateCommitBatchIfNeeded() {
   DCHECK(!is_shutdown_);
   if (!commit_batch_.get()) {
     commit_batch_.reset(new CommitBatch());
 
-    // Start a timer to commit any changes that accrue in the batch,
-    // but only if a commit is not currently in flight. In that case
-    // the timer will be started after the current commit has happened.
-    if (!in_flight_commit_batch_.get()) {
+    // Start a timer to commit any changes that accrue in the batch, but only if
+    // no commits are currently in flight. In that case the timer will be
+    // started after the commits have happened.
+    if (!commit_batches_in_flight_) {
       task_runner_->PostDelayedTask(
           FROM_HERE,
           base::Bind(&DomStorageArea::OnCommitTimer, this),
           base::TimeDelta::FromSeconds(kCommitTimerSeconds));
     }
   }
   return commit_batch_.get();
 }
 
 void DomStorageArea::OnCommitTimer() {
   DCHECK_EQ(kLocalStorageNamespaceId, namespace_id_);
   if (is_shutdown_)
     return;
 
   DCHECK(backing_.get());
   DCHECK(commit_batch_.get());
-  DCHECK(!in_flight_commit_batch_.get());
+  DCHECK(!commit_batches_in_flight_);
 
   // This method executes on the primary sequence, we schedule
   // a task for immediate execution on the commit sequence.
   DCHECK(task_runner_->IsRunningOnPrimarySequence());
-  in_flight_commit_batch_ = commit_batch_.Pass();
+  CommitBatch* in_flight_commit_batch = commit_batch_.release();
   bool success = task_runner_->PostShutdownBlockingTask(
       FROM_HERE,
       DomStorageTaskRunner::COMMIT_SEQUENCE,
-      base::Bind(&DomStorageArea::CommitChanges, this));
+      base::Bind(&DomStorageArea::CommitChanges, this,
+                 *in_flight_commit_batch));

[michaeln, 2012/05/10 16:19:22]

Please use base::Owned<> here to pass ownership of the heap allocated
CommitBatch to the bind closure instead of making a copy of the struct in the
closure. As coded, it looks like the original heap allocated block is being
leaked here. By use base::Owned<>, no leak and no memcpys needed.

[marja, 2012/05/11 07:50:05]

Done.

+  ++commit_batches_in_flight_;
   DCHECK(success);
 }
 
-void DomStorageArea::CommitChanges() {
+void DomStorageArea::CommitChanges(const CommitBatch& commit_batch) {

[michaeln, 2012/05/10 16:19:22]

When using base::Owned<>, this method would take a const CommitBatch* as input.

[marja, 2012/05/11 07:50:05]

Done.

   // This method executes on the commit sequence.
   DCHECK(task_runner_->IsRunningOnCommitSequence());
-  DCHECK(in_flight_commit_batch_.get());
-  bool success = backing_->CommitChanges(
-      in_flight_commit_batch_->clear_all_first,
-      in_flight_commit_batch_->changed_values);
+  bool success = backing_->CommitChanges(commit_batch.clear_all_first,
+                                         commit_batch.changed_values);
   DCHECK(success);  // TODO(michaeln): what if it fails?
   task_runner_->PostTask(
       FROM_HERE,
       base::Bind(&DomStorageArea::OnCommitComplete, this));
 }
 
 void DomStorageArea::OnCommitComplete() {
   // We're back on the primary sequence in this method.
   DCHECK(task_runner_->IsRunningOnPrimarySequence());
   if (is_shutdown_)
     return;
-  in_flight_commit_batch_.reset();
-  if (commit_batch_.get()) {
+  --commit_batches_in_flight_;
+  if (commit_batch_.get() && !commit_batches_in_flight_) {
     // More changes have accrued, restart the timer.
     task_runner_->PostDelayedTask(
         FROM_HERE,
         base::Bind(&DomStorageArea::OnCommitTimer, this),
         base::TimeDelta::FromSeconds(kCommitTimerSeconds));
   }
 }
 
 void DomStorageArea::ShutdownInCommitSequence() {
   // This method executes on the commit sequence.
   DCHECK(task_runner_->IsRunningOnCommitSequence());
   DCHECK(backing_.get());
   if (commit_batch_.get()) {
     // Commit any changes that accrued prior to the timer firing.
     bool success = backing_->CommitChanges(
         commit_batch_->clear_all_first,
         commit_batch_->changed_values);
     DCHECK(success);
   }
   commit_batch_.reset();
-  in_flight_commit_batch_.reset();
   backing_.reset();
 }
 
 }  // namespace dom_storage