From 7ec1259cf9c63e88a15d16babdbbf18872ec8a7e Mon Sep 17 00:00:00 2001 From: Kenneth Russell Date: Fri, 30 Jul 2021 21:43:03 +0000 Subject: [PATCH] Revert "*Scan: Disable *Scan if any of renderers is loading" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit c3b1439ca0fa70a9d872af1c4b396f4436f6ea89. 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 > Reviewed-by: Charlie Reis > Reviewed-by: Michael Lippautz > Reviewed-by: Łukasz Anforowicz > Reviewed-by: Kentaro Hara > 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 Owners-Override: Stephen Martinis Reviewed-by: Stephen Martinis Reviewed-by: ccameron Reviewed-by: Kentaro Hara Commit-Queue: Yuly Novikov Cr-Commit-Position: refs/heads/master@{#907274} --- .../partition_allocator/starscan/pcscan.cc | 15 ----- .../partition_allocator/starscan/pcscan.h | 14 ++-- .../starscan/pcscan_scheduling.cc | 65 ++++--------------- .../starscan/pcscan_scheduling.h | 19 ------ content/browser/BUILD.gn | 2 - content/browser/starscan_load_observer.cc | 64 ------------------ content/browser/starscan_load_observer.h | 48 -------------- .../browser/web_contents/web_contents_impl.cc | 12 ---- .../browser/web_contents/web_contents_impl.h | 4 -- .../web_contents_impl_browsertest.cc | 55 ---------------- 10 files changed, 18 insertions(+), 280 deletions(-) delete mode 100644 content/browser/starscan_load_observer.cc delete mode 100644 content/browser/starscan_load_observer.h diff --git a/base/allocator/partition_allocator/starscan/pcscan.cc b/base/allocator/partition_allocator/starscan/pcscan.cc index daf1050c23849..de8ba54b46388 100644 --- a/base/allocator/partition_allocator/starscan/pcscan.cc +++ b/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); } diff --git a/base/allocator/partition_allocator/starscan/pcscan.h b/base/allocator/partition_allocator/starscan/pcscan.h index ee9f043d84218..5d2aa5aacbc24 100644 --- a/base/allocator/partition_allocator/starscan/pcscan.h +++ b/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(); diff --git a/base/allocator/partition_allocator/starscan/pcscan_scheduling.cc b/base/allocator/partition_allocator/starscan/pcscan_scheduling.cc index e43b61bfc5184..ffae48f4d46e5 100644 --- a/base/allocator/partition_allocator/starscan/pcscan_scheduling.cc +++ b/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. diff --git a/base/allocator/partition_allocator/starscan/pcscan_scheduling.h b/base/allocator/partition_allocator/starscan/pcscan_scheduling.h index 2aad59d6d4176..e9693e7b0841e 100644 --- a/base/allocator/partition_allocator/starscan/pcscan_scheduling.h +++ b/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 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 schedule_delayed_scan_; diff --git a/content/browser/BUILD.gn b/content/browser/BUILD.gn index 2ebfa2974a94f..3778dabd5f563 100644 --- a/content/browser/BUILD.gn +++ b/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", diff --git a/content/browser/starscan_load_observer.cc b/content/browser/starscan_load_observer.cc deleted file mode 100644 index 66adea4cec5f6..0000000000000 --- a/content/browser/starscan_load_observer.cc +++ /dev/null @@ -1,64 +0,0 @@ -// Copyright (c) 2021 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 "content/browser/starscan_load_observer.h" - -#include "base/allocator/partition_allocator/starscan/pcscan.h" -#include "base/logging.h" -#include "content/public/browser/browser_thread.h" - -namespace content { - -namespace { -size_t g_loading_webcontents_ = 0; -} - -// Start observing right away. -StarScanLoadObserver::StarScanLoadObserver(WebContents* contents) - : WebContentsObserver(contents) {} - -StarScanLoadObserver::~StarScanLoadObserver() { - // WebContents can be destructed while loading is still in progress. - DidStopLoading(); -} - -void StarScanLoadObserver::ReadyToCommitNavigation( - NavigationHandle* navigation_handle) { - DCHECK_CURRENTLY_ON(BrowserThread::UI); - // Protect against ReadyToCommitNavigation() being called twice in a row. - if (is_loading_) - return; - is_loading_ = true; - - if (!g_loading_webcontents_) { - VLOG(3) << "Disabling *Scan due to loading"; - base::internal::PCScan::Disable(); - } - ++g_loading_webcontents_; - - // Set timer as a fallback if loading is too slow. - constexpr base::TimeDelta kReenableStarScanDelta = - base::TimeDelta::FromSeconds(10); - timer_.Start(FROM_HERE, kReenableStarScanDelta, this, - &StarScanLoadObserver::DidStopLoading); -} - -void StarScanLoadObserver::DidStopLoading() { - DCHECK_CURRENTLY_ON(BrowserThread::UI); - if (is_loading_) { - DecrementCounterAndReenableStarScanIfNeeded(); - is_loading_ = false; - } -} - -void StarScanLoadObserver::DecrementCounterAndReenableStarScanIfNeeded() { - CHECK(g_loading_webcontents_); - --g_loading_webcontents_; - if (!g_loading_webcontents_) { - VLOG(3) << "Reenabling *Scan after finishing loading"; - base::internal::PCScan::Reenable(); - } -} - -} // namespace content diff --git a/content/browser/starscan_load_observer.h b/content/browser/starscan_load_observer.h deleted file mode 100644 index 8f781bcf62e8d..0000000000000 --- a/content/browser/starscan_load_observer.h +++ /dev/null @@ -1,48 +0,0 @@ -// Copyright (c) 2021 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. - -#ifndef CONTENT_BROWSER_STARSCAN_LOAD_OBSERVER_H_ -#define CONTENT_BROWSER_STARSCAN_LOAD_OBSERVER_H_ - -#include "base/timer/timer.h" -#include "content/public/browser/web_contents_observer.h" - -namespace content { - -// Observes the loading stage of each WebContents and disables *Scan while there -// is at least one WebContents that is being loaded. The approach still -// preserves fallbacks that reenable PCScan: -// - the hard limit of 50% quarantine is reached (see in pcscan_scheduling.h); -// - 10 seconds timer (if there are slow loads). -// TODO(bikineev,1129751): Investigate if a clearer signal to disable *Scan can -// be used instead of WebContentsObserver (e.g. if there is a pending -// USER_BLOCKING task). -// TODO(1231679): Remove/reevaluate the approach. -class StarScanLoadObserver final : public WebContentsObserver { - public: - explicit StarScanLoadObserver(WebContents* contents); - - StarScanLoadObserver(const StarScanLoadObserver&) = delete; - StarScanLoadObserver& operator=(const StarScanLoadObserver&) = delete; - - ~StarScanLoadObserver() override; - - private: - // Disable *Scan when any frame is ready to commit (i.e., has received the - // network response for a navigation) until it finishes loading. - void ReadyToCommitNavigation(NavigationHandle* navigation_handle) final; - void DidStopLoading() final; - - void DecrementCounterAndReenableStarScanIfNeeded(); - - // The current WebContents can be destructed while loading is in progress. - // Keep track of the state with a per WebContents variable. - bool is_loading_ = false; - // Timer is used as a fallback in case loading is too slow. - base::OneShotTimer timer_; -}; - -} // namespace content - -#endif // CONTENT_BROWSER_STARSCAN_LOAD_OBSERVER_H_ diff --git a/content/browser/web_contents/web_contents_impl.cc b/content/browser/web_contents/web_contents_impl.cc index f7a57d2a94ad8..6ccb2a4fe8c50 100644 --- a/content/browser/web_contents/web_contents_impl.cc +++ b/content/browser/web_contents/web_contents_impl.cc @@ -13,7 +13,6 @@ #include #include -#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(this); - } -#endif } WebContentsImpl::~WebContentsImpl() { diff --git a/content/browser/web_contents/web_contents_impl.h b/content/browser/web_contents/web_contents_impl.h index 1f00c94615b6a..5c0f27ff44901 100644 --- a/content/browser/web_contents/web_contents_impl.h +++ b/content/browser/web_contents/web_contents_impl.h @@ -40,7 +40,6 @@ #include "content/browser/renderer_host/render_view_host_delegate.h" #include "content/browser/renderer_host/render_view_host_impl.h" #include "content/browser/renderer_host/render_widget_host_delegate.h" -#include "content/browser/starscan_load_observer.h" #include "content/browser/web_contents/file_chooser_impl.h" #include "content/common/content_export.h" #include "content/public/browser/global_routing_id.h" @@ -2225,9 +2224,6 @@ class CONTENT_EXPORT WebContentsImpl : public WebContents, // color or if the page does not set a background color. absl::optional page_base_background_color_; - // TODO(1231679): Remove/reevaluate after the PCScan experiment is finished. - std::unique_ptr star_scan_load_observer_; - base::WeakPtrFactory loading_weak_factory_{this}; base::WeakPtrFactory weak_factory_{this}; diff --git a/content/browser/web_contents/web_contents_impl_browsertest.cc b/content/browser/web_contents/web_contents_impl_browsertest.cc index 48ee6cc1a7d50..aeebbfb98b459 100644 --- a/content/browser/web_contents/web_contents_impl_browsertest.cc +++ b/content/browser/web_contents/web_contents_impl_browsertest.cc @@ -7,7 +7,6 @@ #include #include -#include "base/allocator/buildflags.h" #include "base/bind.h" #include "base/callback_helpers.h" #include "base/containers/contains.h" @@ -95,11 +94,6 @@ #include "ui/base/clipboard/clipboard_format_type.h" #include "url/gurl.h" -#if BUILDFLAG(USE_PARTITION_ALLOC_AS_MALLOC) -#include "base/allocator/partition_allocator/partition_alloc_features.h" -#include "base/allocator/partition_allocator/starscan/pcscan.h" -#endif - namespace content { #define SCOPE_TRACED(statement) \ @@ -4937,53 +4931,4 @@ IN_PROC_BROWSER_TEST_F(WebContentsImplAllowInsecureLocalhostBrowserTest, observer.Wait(); } -#if BUILDFLAG(USE_PARTITION_ALLOC_AS_MALLOC) && defined(PA_ALLOW_PCSCAN) - -namespace { - -class PCScanFeature { - public: - PCScanFeature() { - scoped_feature_list_.InitAndEnableFeature( - base::features::kPartitionAllocPCScanBrowserOnly); - } - - private: - base::test::ScopedFeatureList scoped_feature_list_; -}; - -// Initialize PCScanFeature before WebContentsImplBrowserTest to make sure that -// the feature is enabled within the entire lifetime of the test. -class WebContentsImplStarScanBrowserTest : private PCScanFeature, - public WebContentsImplBrowserTest {}; - -} // namespace - -IN_PROC_BROWSER_TEST_F(WebContentsImplStarScanBrowserTest, - StarScanDisabledWhileLoadInProgress) { - ASSERT_TRUE(embedded_test_server()->Start()); - - const GURL url = embedded_test_server()->GetURL("/title1.html"); - - TestNavigationManager navigation_manager(shell()->web_contents(), url); - shell()->LoadURL(url); - - // Check that PCScan is initially enabled. - EXPECT_TRUE(base::internal::PCScan::IsEnabled()); - - // Start request and check that PCScan is still enabled. - EXPECT_TRUE(navigation_manager.WaitForRequestStart()); - EXPECT_TRUE(base::internal::PCScan::IsEnabled()); - - // Wait for navigation to finish and check that PCScan is disabled. - navigation_manager.WaitForNavigationFinished(); - EXPECT_FALSE(base::internal::PCScan::IsEnabled()); - - // Complete load and check that PCScan is enabled again. - WaitForLoadStop(shell()->web_contents()); - EXPECT_TRUE(base::internal::PCScan::IsEnabled()); -} - -#endif // BUILDFLAG(USE_PARTITION_ALLOC_AS_MALLOC) && defined(PA_ALLOW_PCSCAN) - } // namespace content