Revert "Implement optimistic writes in remaining paths of blockfile d…

…isk cache."

This reverts commit bb9a602.

Reason for revert: This didn't show an improvement in stable channel.

Original change's description:
> Implement optimistic writes in remaining paths of blockfile disk cache.
>
> This allows the consumer to get the response data without waiting for it to be written to the disk cache. Windows is the only platform not using Simple Cache, which already has this implemented. From local testing, 3/4 of the time writes currently go through the UserBuffer in-memory cache and make a copy, so the data can go to the consumer without waiting to disk. We can do this for the other 1/4 to see if it improves performance.
>
> Bug: 1367943
>
> Change-Id: I0af7e45b6bd190f60ca101a00da3c3e1fd7f30ad
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3878865
> Commit-Queue: John Abd-El-Malek <jam@chromium.org>
> Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org>
> Reviewed-by: Maks Orlovich <morlovich@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1052712}

Bug: 1367943
Change-Id: I3241a395438880907757170393e25318f2748318
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4350974
Reviewed-by: Maks Orlovich <morlovich@chromium.org>
Commit-Queue: John Abd-El-Malek <jam@chromium.org>
Auto-Submit: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1119694}

net/base/features.cc

@@ -251,12 +251,6 @@ BASE_FEATURE(kBlockSetCookieHeader,
              "BlockSetCookieHeader",
              base::FEATURE_ENABLED_BY_DEFAULT);
 
-// Run callbacks optimstically for write calls to the blockfile disk cache
-// implementation.
-BASE_FEATURE(kOptimisticBlockfileWrite,
-             "OptimisticBlockfileWrite",
-             base::FEATURE_DISABLED_BY_DEFAULT);
-
 // Enables partitioning of third party storage (IndexedDB, CacheStorage, etc.)
 // by the top level site to reduce fingerprinting.
 BASE_FEATURE(kThirdPartyStoragePartitioning,

net/base/features.h

@@ -307,8 +307,6 @@ NET_EXPORT BASE_DECLARE_FEATURE(kCookieDomainRejectNonASCII);
 // Blocks the 'Set-Cookie' request header on outbound fetch requests.
 NET_EXPORT BASE_DECLARE_FEATURE(kBlockSetCookieHeader);
 
-NET_EXPORT BASE_DECLARE_FEATURE(kOptimisticBlockfileWrite);
-
 NET_EXPORT BASE_DECLARE_FEATURE(kThirdPartyStoragePartitioning);
 NET_EXPORT BASE_DECLARE_FEATURE(kSupportPartitionedBlobUrl);
 

net/disk_cache/backend_unittest.cc

@@ -284,12 +284,9 @@ int DiskCacheBackendTest::GeneratePendingIO(net::TestCompletionCallback* cb) {
     // We are using the current thread as the cache thread because we want to
     // be able to call directly this method to make sure that the OS (instead
     // of us switching thread) is returning IO pending.
-    bool optimistic = false;
     if (!simple_cache_mode_) {
       rv = static_cast<disk_cache::EntryImpl*>(entry)->WriteDataImpl(
-          0, i, buffer.get(), kSize, cb->callback(), false, &optimistic);
-      if (optimistic)
-        rv = net::ERR_IO_PENDING;
+          0, i, buffer.get(), kSize, cb->callback(), false);
     } else {
       rv = entry->WriteData(0, i, buffer.get(), kSize, cb->callback(), false);
     }

net/disk_cache/blockfile/entry_impl.cc

@@ -9,12 +9,9 @@
 
 #include "base/files/file_util.h"
 #include "base/hash/hash.h"
-#include "base/metrics/histogram_macros.h"
 #include "base/numerics/safe_math.h"
 #include "base/strings/string_util.h"
 #include "base/time/time.h"
-#include "build/build_config.h"
-#include "net/base/features.h"
 #include "net/base/io_buffer.h"
 #include "net/base/net_errors.h"
 #include "net/disk_cache/blockfile/backend_impl.h"
@@ -358,16 +355,15 @@ int EntryImpl::WriteDataImpl(int index,
                              IOBuffer* buf,
                              int buf_len,
                              CompletionOnceCallback callback,
-                             bool truncate,
-                             bool* optimistic) {
+                             bool truncate) {
   if (net_log_.IsCapturing()) {
     NetLogReadWriteData(net_log_, net::NetLogEventType::ENTRY_WRITE_DATA,
                         net::NetLogEventPhase::BEGIN, index, offset, buf_len,
                         truncate);
   }
 
   int result = InternalWriteData(index, offset, buf, buf_len,
-                                 std::move(callback), truncate, optimistic);
+                                 std::move(callback), truncate);
 
   if (result != net::ERR_IO_PENDING && net_log_.IsCapturing()) {
     NetLogReadWriteComplete(net_log_, net::NetLogEventType::ENTRY_WRITE_DATA,
@@ -700,12 +696,10 @@ void EntryImpl::FixForDelete() {
 }
 
 void EntryImpl::IncrementIoCount() {
-  ++io_count_;
   backend_->IncrementIoCount();
 }
 
 void EntryImpl::DecrementIoCount() {
-  --io_count_;
   if (backend_.get())
     backend_->DecrementIoCount();
 }
@@ -879,11 +873,8 @@ int EntryImpl::WriteData(int index,
                          CompletionOnceCallback callback,
                          bool truncate) {
   if (callback.is_null()) {
-    bool optimistic;
-    int err = WriteDataImpl(index, offset, buf, buf_len, std::move(callback),
-                            truncate, &optimistic);
-    DCHECK(!optimistic);
-    return err;
+    return WriteDataImpl(index, offset, buf, buf_len, std::move(callback),
+                         truncate);
   }
 
   DCHECK(node_.Data()->dirty || read_only_);
@@ -1123,11 +1114,9 @@ int EntryImpl::InternalWriteData(int index,
                                  IOBuffer* buf,
                                  int buf_len,
                                  CompletionOnceCallback callback,
-                                 bool truncate,
-                                 bool* optimistic) {
+                                 bool truncate) {
   DCHECK(node_.Data()->dirty || read_only_);
   DVLOG(2) << "Write to " << index << " at " << offset << " : " << buf_len;
-  *optimistic = false;
   if (index < 0 || index >= kNumStreams)
     return net::ERR_INVALID_ARGUMENT;
 
@@ -1166,8 +1155,6 @@ int EntryImpl::InternalWriteData(int index,
   backend_->OnEvent(Stats::WRITE_DATA);
   backend_->OnWrite(buf_len);
 
-  UMA_HISTOGRAM_BOOLEAN("HttpCache.BlockfileWriteInUserBuffer",
-                        !!user_buffers_[index].get());
   if (user_buffers_[index].get()) {
     // Complete the operation locally.
     user_buffers_[index]->Write(offset, buf, buf_len);
@@ -1200,32 +1187,17 @@ int EntryImpl::InternalWriteData(int index,
   if (!buf_len)
     return 0;
 
-  scoped_refptr<net::IOBuffer> op_buf = buf;
-
   SyncCallback* io_callback = nullptr;
   bool null_callback = callback.is_null();
   if (!null_callback) {
-#if BUILDFLAG(IS_WIN)
-    // Only do this optimization on Windows, since it's guaranteed there that an
-    // async write to the File object followed by a read will always give the
-    // previously written data. On Posix this isn't the case.
-    if (base::FeatureList::IsEnabled(
-            net::features::kOptimisticBlockfileWrite) &&
-        io_count_ == 0) {
-      op_buf = base::MakeRefCounted<IOBuffer>(buf_len);
-      memcpy(op_buf->data(), buf->data(), buf_len);
-      *optimistic = true;
-    }
-#endif
-
-    io_callback = new SyncCallback(this, op_buf.get(), std::move(callback),
+    io_callback = new SyncCallback(this, buf, std::move(callback),
                                    net::NetLogEventType::ENTRY_WRITE_DATA);
   }
 
   TimeTicks start_async = TimeTicks::Now();
 
   bool completed;
-  if (!file->Write(op_buf->data(), buf_len, file_offset, io_callback,
+  if (!file->Write(buf->data(), buf_len, file_offset, io_callback,
                    &completed)) {
     if (io_callback)
       io_callback->Discard();
@@ -1239,8 +1211,7 @@ int EntryImpl::InternalWriteData(int index,
     ReportIOTime(kWriteAsync1, start_async);
 
   ReportIOTime(kWrite, start);
-  return (completed || *optimistic || null_callback) ? buf_len
-                                                     : net::ERR_IO_PENDING;
+  return (completed || null_callback) ? buf_len : net::ERR_IO_PENDING;
 }
 
 // ------------------------------------------------------------------------

net/disk_cache/blockfile/entry_impl.h

@@ -64,8 +64,7 @@ class NET_EXPORT_PRIVATE EntryImpl
                     IOBuffer* buf,
                     int buf_len,
                     CompletionOnceCallback callback,
-                    bool truncate,
-                    bool* optimistic);
+                    bool truncate);
   int ReadSparseDataImpl(int64_t offset,
                          IOBuffer* buf,
                          int buf_len,
@@ -224,8 +223,7 @@ class NET_EXPORT_PRIVATE EntryImpl
                         IOBuffer* buf,
                         int buf_len,
                         CompletionOnceCallback callback,
-                        bool truncate,
-                        bool* optimistic);
+                        bool truncate);
 
   // Initializes the storage for an internal or external data block.
   bool CreateDataBlock(int index, int size);
@@ -312,7 +310,6 @@ class NET_EXPORT_PRIVATE EntryImpl
   bool read_only_;            // True if not yet writing.
   bool dirty_ = false;        // True if we detected that this is a dirty entry.
   std::unique_ptr<SparseControl> sparse_;  // Support for sparse entries.
-  int io_count_ = 0;
 };
 
 }  // namespace disk_cache

net/disk_cache/blockfile/in_flight_backend_io.cc

@@ -77,8 +77,7 @@ void BackendIO::OnIOComplete(int result) {
   DCHECK(IsEntryOperation());
   DCHECK_NE(result, net::ERR_IO_PENDING);
   result_ = result;
-  if (notify_controller_)
-    NotifyController();
+  NotifyController();
 }
 
 // Runs on the primary thread.
@@ -391,16 +390,11 @@ void BackendIO::ExecuteEntryOperation() {
           entry_->ReadDataImpl(index_, offset_, buf_.get(), buf_len_,
                                base::BindOnce(&BackendIO::OnIOComplete, this));
       break;
-    case OP_WRITE: {
-      bool optimistic = false;
-      result_ =
-          entry_->WriteDataImpl(index_, offset_, buf_.get(), buf_len_,
-                                base::BindOnce(&BackendIO::OnIOComplete, this),
-                                truncate_, &optimistic);
-      if (optimistic)
-        notify_controller_ = false;
+    case OP_WRITE:
+      result_ = entry_->WriteDataImpl(
+          index_, offset_, buf_.get(), buf_len_,
+          base::BindOnce(&BackendIO::OnIOComplete, this), truncate_);
       break;
-    }
     case OP_READ_SPARSE:
       result_ = entry_->ReadSparseDataImpl(
           offset64_, buf_.get(), buf_len_,

net/disk_cache/blockfile/in_flight_backend_io.h

@@ -183,7 +183,6 @@ class BackendIO : public BackgroundIO {
   bool truncate_ = false;
   int64_t offset64_ = 0;
   base::TimeTicks start_time_;
-  bool notify_controller_ = true;
   base::OnceClosure task_;
 
   scoped_refptr<base::SingleThreadTaskRunner> background_task_runner_;

net/disk_cache/blockfile/sparse_control.cc

@@ -739,7 +739,6 @@ bool SparseControl::DoChildIO() {
   }
 
   int rv = 0;
-  bool optimistic = false;
   switch (operation_) {
     case kReadOperation:
       if (entry_->net_log().IsCapturing()) {
@@ -759,8 +758,7 @@ bool SparseControl::DoChildIO() {
                               child_->net_log().source(), child_len_);
       }
       rv = child_->WriteDataImpl(kSparseData, child_offset_, user_buf_.get(),
-                                 child_len_, std::move(callback), false,
-                                 &optimistic);
+                                 child_len_, std::move(callback), false);
       break;
     case kGetRangeOperation:
       rv = DoGetAvailableRange();
@@ -769,7 +767,7 @@ bool SparseControl::DoChildIO() {
       NOTREACHED();
   }
 
-  if (rv == net::ERR_IO_PENDING || optimistic) {
+  if (rv == net::ERR_IO_PENDING) {
     if (!pending_) {
       pending_ = true;
       // The child will protect himself against closing the entry while IO is in

net/disk_cache/entry_unittest.cc

@@ -19,7 +19,6 @@
 #include "base/time/time.h"
 #include "build/build_config.h"
 #include "net/base/completion_once_callback.h"
-#include "net/base/features.h"
 #include "net/base/io_buffer.h"
 #include "net/base/net_errors.h"
 #include "net/base/request_priority.h"
@@ -5583,59 +5582,6 @@ TEST_F(DiskCacheEntryTest, BlockFileSparsePendingAfterDtor) {
   FlushQueueForTest();
 }
 
-#if BUILDFLAG(IS_WIN)
-TEST_F(DiskCacheEntryTest, BlockFileOptimisticWrite) {
-  base::test::ScopedFeatureList feature_list;
-  feature_list.InitAndEnableFeature(net::features::kOptimisticBlockfileWrite);
-
-  CreateBackend(disk_cache::kNone);
-  disk_cache::Entry* entry = nullptr;
-  ASSERT_THAT(CreateEntry("key", &entry), IsOk());
-  ASSERT_TRUE(entry != nullptr);
-
-  // To ensure `EntryImpl::user_buffers_` aren't used in this test:
-  // 1) make an initial write bigger than kMaxBlockSize
-  // 2) second write has to be bigger than kMaxBlockSize but smaller than the
-  // first so that truncation happens.
-  const int kSize1 = disk_cache::kMaxBlockSize + 100;
-  scoped_refptr<net::IOBuffer> write_buf1 =
-      base::MakeRefCounted<net::IOBuffer>(kSize1);
-  CacheTestFillBuffer(write_buf1->data(), kSize1, false);
-
-  net::TestCompletionCallback cb_write1;
-  EXPECT_EQ(net::ERR_IO_PENDING,
-            entry->WriteData(0, 0, write_buf1.get(), kSize1,
-                             cb_write1.callback(), true));
-  EXPECT_EQ(kSize1, cb_write1.WaitForResult());
-
-  entry->Close();
-
-  ASSERT_THAT(OpenEntry("key", &entry), IsOk());
-  ASSERT_TRUE(entry != nullptr);
-
-  const int kSize2 = disk_cache::kMaxBlockSize + 1;
-  scoped_refptr<net::IOBuffer> write_buf2 =
-      base::MakeRefCounted<net::IOBuffer>(kSize2);
-  CacheTestFillBuffer(write_buf2->data(), kSize2, false);
-
-  net::TestCompletionCallback cb_write2;
-  EXPECT_EQ(net::ERR_IO_PENDING,
-            entry->WriteData(0, 0, write_buf2.get(), kSize2,
-                             cb_write2.callback(), true));
-  EXPECT_EQ(kSize2, cb_write2.WaitForResult());
-
-  scoped_refptr<net::IOBuffer> read_buf =
-      base::MakeRefCounted<net::IOBuffer>(kSize2);
-  net::TestCompletionCallback cb_read;
-  EXPECT_EQ(net::ERR_IO_PENDING,
-            entry->ReadData(0, 0, read_buf.get(), kSize2, cb_read.callback()));
-  EXPECT_EQ(kSize2, cb_read.WaitForResult());
-  EXPECT_EQ(0, memcmp(read_buf->data(), write_buf2->data(), kSize2));
-
-  entry->Close();
-}
-#endif
-
 class DiskCacheSimplePrefetchTest : public DiskCacheEntryTest {
  public:
   DiskCacheSimplePrefetchTest() = default;

testing/variations/fieldtrial_testing_config.json

@@ -8690,21 +8690,6 @@
             ]
         }
     ],
-    "OptimisticBlockfileWrite": [
-        {
-            "platforms": [
-                "windows"
-            ],
-            "experiments": [
-                {
-                    "name": "Enabled",
-                    "enable_features": [
-                        "OptimisticBlockfileWrite"
-                    ]
-                }
-            ]
-        }
-    ],
     "OptimizationGuideInstallWideModelStore": [
         {
             "platforms": [

tools/metrics/histograms/metadata/net/histograms.xml

@@ -225,17 +225,6 @@ chromium-metrics-reviews@google.com.
   </summary>
 </histogram>
 
-<histogram name="HttpCache.BlockfileWriteInUserBuffer" enum="Boolean"
-    expires_after="2023-08-13">
-  <owner>jam@chromium.org</owner>
-  <owner>swarm-team@google.com</owner>
-  <summary>
-    Records whether a write operation in the blockfile implementation used the
-    in-memory user-buffer or went to disk. This is emitted each time a write
-    operation to the disk cache occurs.
-  </summary>
-</histogram>
-
 <histogram name="HttpCache.CreateDiskEntry" units="ms"
     expires_after="2023-08-20">
   <owner>yhirano@chromium.org</owner>