Revert "*Scan: Disable *Scan if any of renderers is loading"

This reverts commit c3b1439.

Reason for revert: likely cause of widespread timeouts http://crbug.com/1234564

Original change's description:
> *Scan: Disable *Scan if any of renderers is loading
>
> In scenarios when a renderer loads a page while *Scan is actively
> scanning the browser heap, it can happen that *Scan takes away all the
> machine cores. This slows down page loading and causes some metrics to
> regress ("First Input Delay", "Time to First Contentful Paint", etc).
>
> This CL introduces StarScanLoadObserver that listens to load changes in
> each WebContents using WebContentsObserver API. In case any of the
> WebContents has renderers that are loading a new document, *Scan in the
> browser process temporarily disables *Scan until all the renderers
> finish loading.
>
> This (intrusive) change is a temporary experiment. Ideally the jobs API
> would be used for scanning tasks, however, due to the cyclic dependency
> between the allocator and the scheduler, it's not trivial to switch to
> jobs API. The bug 1231679 aims to reevaluate the entire approach.
>
> Bug: 1129751,1231679
> Change-Id: Idc75a9d4a8f8bc608cc0934f43948582c0457cd8
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3015346
> Commit-Queue: Anton Bikineev <bikineev@chromium.org>
> Reviewed-by: Charlie Reis <creis@chromium.org>
> Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
> Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
> Reviewed-by: Kentaro Hara <haraken@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#906310}

Bug: 1129751
Bug: 1231679
Bug: 1234564
Change-Id: Ie276a7fcf90b72a4a30c330dc26bc4e47c81eb58
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3063802
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Owners-Override: Stephen Martinis <martiniss@google.com>
Reviewed-by: Stephen Martinis <martiniss@chromium.org>
Reviewed-by: ccameron <ccameron@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Yuly Novikov <ynovikov@chromium.org>
Cr-Commit-Position: refs/heads/master@{#907274}

base/allocator/partition_allocator/starscan/pcscan.cc

@@ -13,21 +13,6 @@ void PCScan::Initialize(WantedWriteProtectionMode wpmode) {
   PCScanInternal::Instance().Initialize(wpmode);
 }
 
-void PCScan::Disable() {
-  auto& instance = PCScan::Instance();
-  instance.scheduler().scheduling_backend().DisableScheduling();
-}
-
-bool PCScan::IsEnabled() {
-  auto& instance = PCScan::Instance();
-  return instance.scheduler().scheduling_backend().is_scheduling_enabled();
-}
-
-void PCScan::Reenable() {
-  auto& instance = PCScan::Instance();
-  instance.scheduler().scheduling_backend().EnableScheduling();
-}
-
 void PCScan::RegisterScannableRoot(Root* root) {
   PCScanInternal::Instance().RegisterScannableRoot(root);
 }

base/allocator/partition_allocator/starscan/pcscan.h

@@ -65,13 +65,6 @@ class BASE_EXPORT PCScan final {
   // Initializes PCScan and prepares internal data structures.
   static void Initialize(WantedWriteProtectionMode);
 
-  // Disable/reenable PCScan. Temporal disabling can be useful in CPU demanding
-  // contexts.
-  static void Disable();
-  static void Reenable();
-  // Query if PCScan is enabled.
-  static bool IsEnabled();
-
   // Registers a root for scanning.
   static void RegisterScannableRoot(Root* root);
   // Registers a root that doesn't need to be scanned but still contains
@@ -87,11 +80,9 @@ class BASE_EXPORT PCScan final {
                                              size_t usable_size,
                                              size_t slot_size);
 
-  // Performs scanning unconditionally.
-  static void PerformScan(InvocationMode invocation_mode);
   // Performs scanning only if a certain quarantine threshold was reached.
   static void PerformScanIfNeeded(InvocationMode invocation_mode);
-  // Performs scanning with specified delay.
+
   static void PerformDelayedScan(TimeDelta delay);
 
   // Join scan from safepoint in mutator thread. As soon as PCScan is scheduled,
@@ -145,6 +136,9 @@ class BASE_EXPORT PCScan final {
 
   inline constexpr PCScan();
 
+  // Performs scanning unconditionally.
+  void PerformScan(InvocationMode invocation_mode);
+
   // Joins scan unconditionally.
   static void JoinScan();
 

base/allocator/partition_allocator/starscan/pcscan_scheduling.cc

@@ -9,7 +9,6 @@
 
 #include "base/allocator/partition_allocator/partition_alloc_check.h"
 #include "base/allocator/partition_allocator/partition_alloc_hooks.h"
-#include "base/allocator/partition_allocator/starscan/pcscan.h"
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/synchronization/lock.h"
@@ -27,17 +26,6 @@ void PCScanScheduler::SetNewSchedulingBackend(
   backend_ = &backend;
 }
 
-void PCScanSchedulingBackend::DisableScheduling() {
-  scheduling_enabled_.store(false, std::memory_order_relaxed);
-}
-
-void PCScanSchedulingBackend::EnableScheduling() {
-  scheduling_enabled_.store(true, std::memory_order_relaxed);
-  // Check if *Scan needs to be run immediately.
-  if (NeedsToImmediatelyScan())
-    PCScan::PerformScan(PCScan::InvocationMode::kNonBlocking);
-}
-
 size_t PCScanSchedulingBackend::ScanStarted() {
   auto& data = GetQuarantineData();
   data.epoch.fetch_add(1, std::memory_order_relaxed);
@@ -52,7 +40,7 @@ TimeDelta PCScanSchedulingBackend::UpdateDelayedSchedule() {
 constexpr double LimitBackend::kQuarantineSizeFraction;
 
 bool LimitBackend::LimitReached() {
-  return is_scheduling_enabled();
+  return true;
 }
 
 void LimitBackend::UpdateScheduleAfterScan(size_t survived_bytes,
@@ -68,10 +56,6 @@ void LimitBackend::UpdateScheduleAfterScan(size_t survived_bytes,
       std::memory_order_relaxed);
 }
 
-bool LimitBackend::NeedsToImmediatelyScan() {
-  return false;
-}
-
 // static
 constexpr double MUAwareTaskBasedBackend::kSoftLimitQuarantineSizePercent;
 // static
@@ -101,21 +85,26 @@ bool MUAwareTaskBasedBackend::LimitReached() {
     data.size_limit.store(hard_limit_, std::memory_order_relaxed);
     hard_limit_ = 0;
 
-    // 2. Unlikely case: If also above hard limit, start scan right away. This
-    // ignores explicit PCScan disabling.
+    // 2. Unlikely case: If also above hard limit, start scan right away.
     if (UNLIKELY(data.current_size.load(std::memory_order_relaxed) >
                  data.size_limit.load(std::memory_order_relaxed))) {
       return true;
     }
 
-    // 3. Check if PCScan was explicitly disabled.
-    if (UNLIKELY(!is_scheduling_enabled())) {
-      return false;
+    // 3. Otherwise, the soft limit would trigger a scan immediately if the
+    // mutator utilization requirement is satisfied.
+    const auto delay = earliest_next_scan_time_ - base::TimeTicks::Now();
+    if (delay <= TimeDelta()) {
+      // May invoke scan immediately.
+      return true;
     }
 
-    // 4. Finally, check if the mutator utilization requirement is satisfied.
-    // Otherwise, reschedule scan.
-    return CheckIfScanIsNeededOrReschedule();
+    VLOG(3) << "Rescheduling scan with delay: " << delay.InMillisecondsF()
+            << " ms";
+    // 4. If the MU requirement is not satisfied, schedule a delayed scan to the
+    // time instance when MU is satisfied.
+    schedule_delayed_scan_.Run(delay);
+    return false;
   }
   return true;
 }
@@ -155,32 +144,6 @@ void MUAwareTaskBasedBackend::UpdateScheduleAfterScan(
   earliest_next_scan_time_ = base::TimeTicks::Now() + time_required_on_mutator;
 }
 
-bool MUAwareTaskBasedBackend::NeedsToImmediatelyScan() {
-  base::AutoLock guard(scheduler_lock_);
-  // If |hard_limit_| was set to zero, the soft limit was reached. Bail out if
-  // it's not.
-  if (hard_limit_)
-    return false;
-  return CheckIfScanIsNeededOrReschedule();
-}
-
-bool MUAwareTaskBasedBackend::CheckIfScanIsNeededOrReschedule() {
-  // Check if the mutator utilization requirement is satisfied and scan can be
-  // triggered immediately.
-  const auto delay = earliest_next_scan_time_ - base::TimeTicks::Now();
-  if (delay <= TimeDelta()) {
-    // May invoke scan immediately.
-    return true;
-  }
-
-  VLOG(3) << "Rescheduling scan with delay: " << delay.InMillisecondsF()
-          << " ms";
-  // If the MU requirement is not satisfied, schedule a delayed scan to the time
-  // instance when MU is satisfied.
-  schedule_delayed_scan_.Run(delay);
-  return false;
-}
-
 TimeDelta MUAwareTaskBasedBackend::UpdateDelayedSchedule() {
   base::AutoLock guard(scheduler_lock_);
   // TODO(1197479): Adjust schedule to current heap sizing.

base/allocator/partition_allocator/starscan/pcscan_scheduling.h

@@ -43,13 +43,6 @@ class BASE_EXPORT PCScanSchedulingBackend {
   PCScanSchedulingBackend(const PCScanSchedulingBackend&) = delete;
   PCScanSchedulingBackend& operator=(const PCScanSchedulingBackend&) = delete;
 
-  void DisableScheduling();
-  void EnableScheduling();
-
-  bool is_scheduling_enabled() const {
-    return scheduling_enabled_.load(std::memory_order_relaxed);
-  }
-
   inline QuarantineData& GetQuarantineData();
 
   // Invoked when the limit in PCScanScheduler is reached. Returning true
@@ -69,12 +62,7 @@ class BASE_EXPORT PCScanSchedulingBackend {
   virtual TimeDelta UpdateDelayedSchedule();
 
  protected:
-  inline bool SchedulingDisabled() const;
-
-  virtual bool NeedsToImmediatelyScan() = 0;
-
   PCScanScheduler& scheduler_;
-  std::atomic<bool> scheduling_enabled_{true};
 };
 
 // Scheduling backend that just considers a single hard limit.
@@ -86,9 +74,6 @@ class BASE_EXPORT LimitBackend final : public PCScanSchedulingBackend {
 
   bool LimitReached() final;
   void UpdateScheduleAfterScan(size_t, base::TimeDelta, size_t) final;
-
- private:
-  bool NeedsToImmediatelyScan() final;
 };
 
 // Task based backend that is aware of a target mutator utilization that
@@ -123,10 +108,6 @@ class BASE_EXPORT MUAwareTaskBasedBackend final
   // memory management in scan.
   static constexpr double kTargetMutatorUtilizationPercent = 0.90;
 
-  bool NeedsToImmediatelyScan() final;
-  bool CheckIfScanIsNeededOrReschedule()
-      EXCLUSIVE_LOCKS_REQUIRED(scheduler_lock_);
-
   // Callback to schedule a delayed scan.
   const base::RepeatingCallback<void(TimeDelta)> schedule_delayed_scan_;
 

content/browser/BUILD.gn

@@ -1827,8 +1827,6 @@ source_set("browser") {
     "ssl/ssl_manager.h",
     "ssl_private_key_impl.cc",
     "ssl_private_key_impl.h",
-    "starscan_load_observer.cc",
-    "starscan_load_observer.h",
     "startup_data_impl.cc",
     "startup_data_impl.h",
     "startup_helper.cc",

content/browser/web_contents/web_contents_impl.cc

@@ -13,7 +13,6 @@
 #include <utility>
 #include <vector>
 
-#include "base/allocator/partition_allocator/partition_alloc_features.h"
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/containers/contains.h"
@@ -91,7 +90,6 @@
 #include "content/browser/screen_enumeration/screen_change_monitor.h"
 #include "content/browser/screen_orientation/screen_orientation_provider.h"
 #include "content/browser/site_instance_impl.h"
-#include "content/browser/starscan_load_observer.h"
 #include "content/browser/wake_lock/wake_lock_context_host.h"
 #include "content/browser/web_contents/java_script_dialog_commit_deferring_condition.h"
 #include "content/browser/web_contents/web_contents_view_child_frame.h"
@@ -100,7 +98,6 @@
 #include "content/browser/webui/web_ui_impl.h"
 #include "content/browser/xr/service/xr_runtime_manager_impl.h"
 #include "content/common/content_switches_internal.h"
-#include "content/common/partition_alloc_support.h"
 #include "content/public/browser/ax_inspect_factory.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_plugin_guest_manager.h"
@@ -900,15 +897,6 @@ WebContentsImpl::WebContentsImpl(BrowserContext* browser_context)
   if (base::FeatureList::IsEnabled(blink::features::kConversionMeasurement)) {
     ConversionHost::CreateForWebContents(this);
   }
-
-#if BUILDFLAG(USE_PARTITION_ALLOC_AS_MALLOC) && defined(PA_ALLOW_PCSCAN)
-  // TODO(1231679): Remove or move to another place after finishing the PCScan
-  // experiment.
-  if (base::FeatureList::IsEnabled(
-          base::features::kPartitionAllocPCScanBrowserOnly)) {
-    star_scan_load_observer_ = std::make_unique<StarScanLoadObserver>(this);
-  }
-#endif
 }
 
 WebContentsImpl::~WebContentsImpl() {