Experiment with disabling BlobMemoryController memory pressure response.

Since this intervention is meant to ease resource use, having it enabled
should show a better performance profile then having it disabled.

The motivation for the experimentation comes from finding cases of
severe resource crunch triggered by the intervention at the critical
pressure level before it was disabled.


Bug: 1087530
Change-Id: I44e6f8351ea6d353328fc9fdf7c416b46f962380
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2227041
Commit-Queue: Oliver Li <olivierli@chromium.org>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#777440}

storage/browser/blob/blob_memory_controller.cc

@@ -14,6 +14,7 @@
 #include "base/callback_helpers.h"
 #include "base/command_line.h"
 #include "base/containers/small_map.h"
+#include "base/feature_list.h"
 #include "base/files/file_util.h"
 #include "base/guid.h"
 #include "base/location.h"
@@ -40,6 +41,13 @@ using base::File;
 using base::FilePath;
 
 namespace storage {
+
+// static
+const base::Feature
+    BlobMemoryController::kInhibitBlobMemoryControllerMemoryPressureResponse{
+        "InhibitBlobMemoryControllerMemoryPressureResponse",
+        base::FEATURE_DISABLED_BY_DEFAULT};
+
 namespace {
 constexpr int64_t kUnknownDiskAvailability = -1ll;
 constexpr uint64_t kMegabyte = 1024ull * 1024;
@@ -1049,6 +1057,11 @@ void BlobMemoryController::OnMemoryPressure(
     return;
   }
 
+  if (base::FeatureList::IsEnabled(
+          kInhibitBlobMemoryControllerMemoryPressureResponse)) {
+    return;
+  }
+
   // TODO(crbug.com/1087530): Run trial to see if we should get rid of this
   // whole intervention or leave it on for MEMORY_PRESSURE_LEVEL_MODERATE.
 

storage/browser/blob/blob_memory_controller.h

@@ -20,6 +20,7 @@
 #include "base/callback_helpers.h"
 #include "base/component_export.h"
 #include "base/containers/mru_cache.h"
+#include "base/feature_list.h"
 #include "base/files/file.h"
 #include "base/files/file_path.h"
 #include "base/gtest_prod_util.h"
@@ -54,6 +55,8 @@ class ShareableFileReference;
 // This class can only be interacted with on the IO thread.
 class COMPONENT_EXPORT(STORAGE_BROWSER) BlobMemoryController {
  public:
+  static const base::Feature kInhibitBlobMemoryControllerMemoryPressureResponse;
+
   enum class Strategy {
     // We don't have enough memory for this blob.
     TOO_LARGE,

storage/browser/blob/blob_memory_controller_unittest.cc

@@ -12,6 +12,7 @@
 #include "base/system/sys_info.h"
 #include "base/test/task_environment.h"
 #include "base/test/test_simple_task_runner.h"
+#include "base/test/with_feature_override.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "storage/browser/blob/blob_data_builder.h"
@@ -47,9 +48,13 @@ int64_t FakeDiskSpaceMethod(const base::FilePath& path) {
   return sFakeDiskSpace;
 }
 
-class BlobMemoryControllerTest : public testing::Test {
+class BlobMemoryControllerTest : public base::test::WithFeatureOverride,
+                                 public testing::Test {
  protected:
-  BlobMemoryControllerTest() = default;
+  BlobMemoryControllerTest()
+      : base::test::WithFeatureOverride(
+            BlobMemoryController::
+                kInhibitBlobMemoryControllerMemoryPressureResponse) {}
 
   void SetUp() override {
     ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
@@ -168,7 +173,7 @@ class BlobMemoryControllerTest : public testing::Test {
   base::test::SingleThreadTaskEnvironment task_environment_;
 };
 
-TEST_F(BlobMemoryControllerTest, Strategy) {
+TEST_P(BlobMemoryControllerTest, Strategy) {
   {
     BlobMemoryController controller(temp_dir_.GetPath(), nullptr);
     SetTestMemoryLimits(&controller);
@@ -226,7 +231,7 @@ TEST_F(BlobMemoryControllerTest, Strategy) {
   }
 }
 
-TEST_F(BlobMemoryControllerTest, GrantMemory) {
+TEST_P(BlobMemoryControllerTest, GrantMemory) {
   const std::string kId = "id";
   BlobMemoryController controller(temp_dir_.GetPath(), file_runner_);
   SetTestMemoryLimits(&controller);
@@ -249,7 +254,7 @@ TEST_F(BlobMemoryControllerTest, GrantMemory) {
   EXPECT_TRUE(HasMemoryAllocation(items[0].get()));
 }
 
-TEST_F(BlobMemoryControllerTest, SimpleMemoryRequest) {
+TEST_P(BlobMemoryControllerTest, SimpleMemoryRequest) {
   const std::string kId = "id";
   BlobMemoryController controller(temp_dir_.GetPath(), file_runner_);
   SetTestMemoryLimits(&controller);
@@ -275,7 +280,7 @@ TEST_F(BlobMemoryControllerTest, SimpleMemoryRequest) {
   EXPECT_EQ(0u, controller.memory_usage());
 }
 
-TEST_F(BlobMemoryControllerTest, PageToDisk) {
+TEST_P(BlobMemoryControllerTest, PageToDisk) {
   const std::string kId = "id";
   const std::string kId2 = "id2";
   BlobMemoryController controller(temp_dir_.GetPath(), file_runner_);
@@ -347,7 +352,7 @@ TEST_F(BlobMemoryControllerTest, PageToDisk) {
   EXPECT_EQ(0u, controller.disk_usage());
 }
 
-TEST_F(BlobMemoryControllerTest, NoDiskTooLarge) {
+TEST_P(BlobMemoryControllerTest, NoDiskTooLarge) {
   BlobMemoryController controller(temp_dir_.GetPath(), nullptr);
   SetTestMemoryLimits(&controller);
 
@@ -356,14 +361,14 @@ TEST_F(BlobMemoryControllerTest, NoDiskTooLarge) {
                                           1));
 }
 
-TEST_F(BlobMemoryControllerTest, TooLargeForDisk) {
+TEST_P(BlobMemoryControllerTest, TooLargeForDisk) {
   BlobMemoryController controller(temp_dir_.GetPath(), file_runner_);
   SetTestMemoryLimits(&controller);
 
   EXPECT_FALSE(controller.CanReserveQuota(kTestBlobStorageMaxDiskSpace + 1));
 }
 
-TEST_F(BlobMemoryControllerTest, CancelMemoryRequest) {
+TEST_P(BlobMemoryControllerTest, CancelMemoryRequest) {
   const std::string kId = "id";
   const std::string kId2 = "id2";
   BlobMemoryController controller(temp_dir_.GetPath(), file_runner_);
@@ -423,7 +428,7 @@ TEST_F(BlobMemoryControllerTest, CancelMemoryRequest) {
   EXPECT_EQ(0u, controller.disk_usage());
 }
 
-TEST_F(BlobMemoryControllerTest, FileRequest) {
+TEST_P(BlobMemoryControllerTest, FileRequest) {
   const std::string kId = "id";
   const size_t kBlobSize = kTestBlobStorageMaxBlobMemorySize + 1;
 
@@ -479,7 +484,7 @@ TEST_F(BlobMemoryControllerTest, FileRequest) {
   EXPECT_EQ(0u, controller.disk_usage());
 }
 
-TEST_F(BlobMemoryControllerTest, CancelFileRequest) {
+TEST_P(BlobMemoryControllerTest, CancelFileRequest) {
   const std::string kId = "id";
   const size_t kBlobSize = kTestBlobStorageMaxBlobMemorySize + 1;
 
@@ -512,7 +517,7 @@ TEST_F(BlobMemoryControllerTest, CancelFileRequest) {
   base::RunLoop().RunUntilIdle();
 }
 
-TEST_F(BlobMemoryControllerTest, MultipleFilesPaged) {
+TEST_P(BlobMemoryControllerTest, MultipleFilesPaged) {
   const std::string kId1 = "id";
   const size_t kSize1 = kTestBlobStorageMaxFileSizeBytes;
   char kData1[kSize1];
@@ -605,7 +610,7 @@ TEST_F(BlobMemoryControllerTest, MultipleFilesPaged) {
   EXPECT_EQ(0u, controller.disk_usage());
 }
 
-TEST_F(BlobMemoryControllerTest, FullEviction) {
+TEST_P(BlobMemoryControllerTest, FullEviction) {
   BlobMemoryController controller(temp_dir_.GetPath(), file_runner_);
   SetTestMemoryLimits(&controller);
   AssertEnoughDiskSpace();
@@ -660,7 +665,7 @@ TEST_F(BlobMemoryControllerTest, FullEviction) {
   EXPECT_TRUE(memory_quota_result_);
 }
 
-TEST_F(BlobMemoryControllerTest, PagingStopsWhenFull) {
+TEST_P(BlobMemoryControllerTest, PagingStopsWhenFull) {
   BlobMemoryController controller(temp_dir_.GetPath(), file_runner_);
   SetTestMemoryLimits(&controller);
   AssertEnoughDiskSpace();
@@ -786,7 +791,7 @@ TEST_F(BlobMemoryControllerTest, PagingStopsWhenFull) {
   EXPECT_EQ(Strategy::TOO_LARGE, controller.DetermineStrategy(1u, 1ull));
 }
 
-TEST_F(BlobMemoryControllerTest, DisableDiskWithFileAndMemoryPending) {
+TEST_P(BlobMemoryControllerTest, DisableDiskWithFileAndMemoryPending) {
   const std::string kFirstMemoryId = "id";
   const uint64_t kFirstMemorySize = kTestBlobStorageMaxBlobMemorySize;
   const std::string kSecondMemoryId = "id2";
@@ -871,7 +876,7 @@ TEST_F(BlobMemoryControllerTest, DisableDiskWithFileAndMemoryPending) {
   EXPECT_EQ(0ull, controller.memory_usage());
 }
 
-TEST_F(BlobMemoryControllerTest, DiskSpaceTooSmallForItem) {
+TEST_P(BlobMemoryControllerTest, DiskSpaceTooSmallForItem) {
   const std::string kFileId = "id2";
   const uint64_t kFileBlobSize = kTestBlobStorageMaxBlobMemorySize;
 
@@ -905,7 +910,7 @@ TEST_F(BlobMemoryControllerTest, DiskSpaceTooSmallForItem) {
   EXPECT_EQ(0ull, controller.memory_usage());
 }
 
-TEST_F(BlobMemoryControllerTest, DiskSpaceHitMinAvailable) {
+TEST_P(BlobMemoryControllerTest, DiskSpaceHitMinAvailable) {
   const std::string kFileId = "id2";
   const uint64_t kFileBlobSize = kTestBlobStorageMaxBlobMemorySize;
 
@@ -947,7 +952,7 @@ TEST_F(BlobMemoryControllerTest, DiskSpaceHitMinAvailable) {
   EXPECT_EQ(0ull, controller.memory_usage());
 }
 
-TEST_F(BlobMemoryControllerTest, DiskSpaceBeforeMinAvailable) {
+TEST_P(BlobMemoryControllerTest, DiskSpaceBeforeMinAvailable) {
   const std::string kFileId = "id2";
 
   BlobMemoryController controller(temp_dir_.GetPath(), file_runner_);
@@ -988,7 +993,7 @@ TEST_F(BlobMemoryControllerTest, DiskSpaceBeforeMinAvailable) {
   EXPECT_EQ(0ull, controller.memory_usage());
 }
 
-TEST_F(BlobMemoryControllerTest, DiskSpaceNearMinAvailable) {
+TEST_P(BlobMemoryControllerTest, DiskSpaceNearMinAvailable) {
   const std::string kFileId = "id2";
 
   BlobMemoryController controller(temp_dir_.GetPath(), file_runner_);
@@ -1030,7 +1035,7 @@ TEST_F(BlobMemoryControllerTest, DiskSpaceNearMinAvailable) {
   EXPECT_EQ(0ull, controller.memory_usage());
 }
 
-TEST_F(BlobMemoryControllerTest, DiskSpaceResetAfterIncrease) {
+TEST_P(BlobMemoryControllerTest, DiskSpaceResetAfterIncrease) {
   const std::string kFileId = "id2";
   const uint64_t kFileBlobSize = kTestBlobStorageMaxBlobMemorySize;
 
@@ -1098,7 +1103,7 @@ TEST_F(BlobMemoryControllerTest, DiskSpaceResetAfterIncrease) {
   EXPECT_EQ(0ull, controller.memory_usage());
 }
 
-TEST_F(BlobMemoryControllerTest, DiskSpaceUnknown) {
+TEST_P(BlobMemoryControllerTest, DiskSpaceUnknown) {
   const std::string kFileId = "id2";
   const uint64_t kFileBlobSize = kTestBlobStorageMaxBlobMemorySize;
 
@@ -1127,7 +1132,7 @@ TEST_F(BlobMemoryControllerTest, DiskSpaceUnknown) {
   EXPECT_FALSE(controller.limits().IsDiskSpaceConstrained());
 }
 
-TEST_F(BlobMemoryControllerTest, OnMemoryPressure) {
+TEST_P(BlobMemoryControllerTest, OnMemoryPressure) {
   BlobMemoryController controller(temp_dir_.GetPath(), file_runner_);
   SetTestMemoryLimits(&controller);
   AssertEnoughDiskSpace();
@@ -1154,24 +1159,36 @@ TEST_F(BlobMemoryControllerTest, OnMemoryPressure) {
   EXPECT_FALSE(file_runner_->HasPendingTask());
   EXPECT_EQ(size_to_load, controller.memory_usage());
 
+  const size_t memory_usage_before_eviction = controller.memory_usage();
+  const size_t disk_usage_before_eviction = controller.disk_usage();
+
   controller.OnMemoryPressure(
       base::MemoryPressureListener::MemoryPressureLevel::
           MEMORY_PRESSURE_LEVEL_MODERATE);
 
-  EXPECT_TRUE(file_runner_->HasPendingTask());
-  RunFileThreadTasks();
+  // Tasks are only posted if the memory pressure response is active.
+  const bool memory_pressure_response_active = !base::FeatureList::IsEnabled(
+      BlobMemoryController::kInhibitBlobMemoryControllerMemoryPressureResponse);
+  EXPECT_EQ(file_runner_->HasPendingTask(), memory_pressure_response_active);
 
-  base::RunLoop().RunUntilIdle();
+  if (memory_pressure_response_active) {
+    RunFileThreadTasks();
 
-  // 2 page files of size |kTestBlobStorageMaxBlobMemorySize *
-  // kTestMaxBlobInMemorySpaceUnderPressureRatio| should be evicted with 1 byte
-  // left in-memory.
-  EXPECT_EQ(1u, controller.memory_usage());
-  EXPECT_EQ(size_to_load - 1, controller.disk_usage());
-  return;
+    base::RunLoop().RunUntilIdle();
+
+    // 2 page files of size |kTestBlobStorageMaxBlobMemorySize *
+    // kTestMaxBlobInMemorySpaceUnderPressureRatio| should be evicted with 1
+    // byte left in-memory.
+    EXPECT_EQ(1u, controller.memory_usage());
+    EXPECT_EQ(size_to_load - 1, controller.disk_usage());
+  } else {
+    // No intervention means the memory usage and disk usage is unchanged.
+    EXPECT_EQ(memory_usage_before_eviction, controller.memory_usage());
+    EXPECT_EQ(disk_usage_before_eviction, controller.disk_usage());
+  }
 }
 
-TEST_F(BlobMemoryControllerTest, LowMemoryDevice) {
+TEST_P(BlobMemoryControllerTest, LowMemoryDevice) {
   BlobMemoryController controller(temp_dir_.GetPath(), nullptr);
   // Make 1% of physical memory size just less than min_page_file_size
   controller.set_amount_of_physical_memory_for_testing(
@@ -1182,4 +1199,6 @@ TEST_F(BlobMemoryControllerTest, LowMemoryDevice) {
   EXPECT_TRUE(controller.limits().IsValid());
 }
 
+INSTANTIATE_FEATURE_OVERRIDE_TEST_SUITE(BlobMemoryControllerTest);
+
 }  // namespace storage

testing/buildbot/filters/fuchsia.storage_unittests.filter

@@ -1,9 +1,15 @@
 # AmountOfFreeDiskSpace not implemented for Fuchsia.
--BlobMemoryControllerTest.FullEviction
--BlobMemoryControllerTest.MultipleFilesPaged
--BlobMemoryControllerTest.OnMemoryPressure
--BlobMemoryControllerTest.PageToDisk
--BlobMemoryControllerTest.PagingStopsWhenFull
+-All/BlobMemoryControllerTest.FullEviction/0
+-All/BlobMemoryControllerTest.MultipleFilesPaged/0
+-All/BlobMemoryControllerTest.OnMemoryPressure/0
+-All/BlobMemoryControllerTest.PageToDisk/0
+-All/BlobMemoryControllerTest.PagingStopsWhenFull/0
+
+-All/BlobMemoryControllerTest.FullEviction/1
+-All/BlobMemoryControllerTest.MultipleFilesPaged/1
+-All/BlobMemoryControllerTest.OnMemoryPressure/1
+-All/BlobMemoryControllerTest.PageToDisk/1
+-All/BlobMemoryControllerTest.PagingStopsWhenFull/1
 
 # crbug.com/1077456
 -All/ObfuscatedFileUtilTest.TestTouch/0
@@ -11,4 +17,4 @@
 -FileSystemOperationImplTest.TestTouchFile
 -LocalFileUtilTest.TouchDirectory
 -LocalFileUtilTest.TouchFile
--NativeFileUtilTest.TouchFileAndGetFileInfo
+-NativeFileUtilTest.TouchFileAndGetFileInfo