DomStorageArea: Allow multiple simultaneous in flight commit batches.

webkit/dom_storage/dom_storage_area.cc

Index: webkit/dom_storage/dom_storage_area.cc
diff --git a/webkit/dom_storage/dom_storage_area.cc b/webkit/dom_storage/dom_storage_area.cc
index f589d88987cc54a7d9079fd00cfc6b16f926c6c0..9f658a3a34ccbb4dbbd48c03298dfded88285ff0 100644
--- a/webkit/dom_storage/dom_storage_area.cc
+++ b/webkit/dom_storage/dom_storage_area.cc
@@ -60,7 +60,8 @@ DomStorageArea::DomStorageArea(
       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));
@@ -161,7 +162,7 @@ DomStorageArea* DomStorageArea::ShallowCopy(int64 destination_namespace_id) {
 
 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() {
@@ -233,10 +234,10 @@ DomStorageArea::CommitBatch* DomStorageArea::CreateCommitBatchIfNeeded() {
   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),
@@ -253,26 +254,26 @@ void DomStorageArea::OnCommitTimer() {
 
   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,
@@ -284,8 +285,8 @@ void DomStorageArea::OnCommitComplete() {
   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,
@@ -306,7 +307,6 @@ void DomStorageArea::ShutdownInCommitSequence() {
     DCHECK(success);
   }
   commit_batch_.reset();
-  in_flight_commit_batch_.reset();
   backing_.reset();
 }