SwapThrashingMonitor: Move the blocking calls to a MayBlock sequence

Bug: 771478
Change-Id: I4454ca4a174775647546432c69bf213bc1b69789
Reviewed-on: https://chromium-review.googlesource.com/c/1455337Reviewed-by: Chris Hamilton <chrisha@chromium.org>
Commit-Queue: Sébastien Marchand <sebmarchand@chromium.org>
Cr-Commit-Position: refs/heads/master@{#630076}

chrome/browser/memory/swap_thrashing_monitor.cc

@@ -7,6 +7,7 @@
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/task_runner_util.h"
 #include "base/time/time.h"
 
 #if defined(OS_WIN)
@@ -69,6 +70,14 @@ SwapThrashingLevelUMA SwapThrashingLevelToUmaEnumValue(
   return UMA_SWAP_THRASHING_LEVEL_NONE;
 }
 
+std::unique_ptr<SwapThrashingMonitorDelegate> GetPlatformSpecificDelegate() {
+#if defined(OS_WIN)
+  return std::make_unique<SwapThrashingMonitorDelegateWin>();
+#else
+  return std::make_unique<SwapThrashingMonitorDelegate>();
+#endif
+}
+
 }  // namespace
 
 // static
@@ -82,14 +91,12 @@ SwapThrashingMonitor* SwapThrashingMonitor::GetInstance() {
 }
 
 SwapThrashingMonitor::SwapThrashingMonitor()
-    : current_swap_thrashing_level_(
-          SwapThrashingLevel::SWAP_THRASHING_LEVEL_NONE) {
+    : delegate_(GetPlatformSpecificDelegate().release(),
+                base::OnTaskRunnerDeleter(blocking_task_runner_)),
+      current_swap_thrashing_level_(
+          SwapThrashingLevel::SWAP_THRASHING_LEVEL_NONE),
+      weak_factory_(this) {
   DCHECK(base::FeatureList::IsEnabled(features::kSwapThrashingMonitor));
-#if defined(OS_WIN)
-  delegate_ = std::make_unique<SwapThrashingMonitorDelegateWin>();
-#else
-  delegate_ = std::make_unique<SwapThrashingMonitorDelegate>();
-#endif
   StartObserving();
 }
 
@@ -105,8 +112,32 @@ SwapThrashingLevel SwapThrashingMonitor::GetCurrentSwapThrashingLevel() {
 void SwapThrashingMonitor::CheckSwapThrashingPressureAndRecordStatistics() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  SwapThrashingLevel swap_thrashing_level =
-      delegate_->SampleAndCalculateSwapThrashingLevel();
+  base::PostTaskAndReplyWithResult(
+      blocking_task_runner_.get(), FROM_HERE,
+      base::BindOnce(
+          &SwapThrashingMonitorDelegate::SampleAndCalculateSwapThrashingLevel,
+          base::Unretained(delegate_.get())),
+      base::BindOnce(&SwapThrashingMonitor::RecordSwapThrashingLevel,
+                     weak_factory_.GetWeakPtr()));
+}
+
+void SwapThrashingMonitor::StartObserving() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  // TODO(sebmarchand): Determine if the system is using on-disk swap, abort if
+  // it isn't as there won't be any swap-paging to observe (on-disk swap could
+  // later become available if the user turn it on but this case is rare that
+  // it's safe to ignore it). See crbug.com/779309.
+  check_timer_.Start(
+      FROM_HERE, kSamplingInterval,
+      base::BindRepeating(
+          &SwapThrashingMonitor::CheckSwapThrashingPressureAndRecordStatistics,
+          base::Unretained(this)));
+}
+
+void SwapThrashingMonitor::RecordSwapThrashingLevel(
+    SwapThrashingLevel swap_thrashing_level) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   // Record the state changes.
   if (swap_thrashing_level != current_swap_thrashing_level_) {
@@ -150,18 +181,4 @@ void SwapThrashingMonitor::CheckSwapThrashingPressureAndRecordStatistics() {
       UMA_SWAP_THRASHING_LEVEL_COUNT);
 }
 
-void SwapThrashingMonitor::StartObserving() {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-
-  // TODO(sebmarchand): Determine if the system is using on-disk swap, abort if
-  // it isn't as there won't be any swap-paging to observe (on-disk swap could
-  // later become available if the user turn it on but this case is rare that
-  // it's safe to ignore it). See crbug.com/779309.
-  check_timer_.Start(
-      FROM_HERE, kSamplingInterval,
-      base::Bind(
-          &SwapThrashingMonitor::CheckSwapThrashingPressureAndRecordStatistics,
-          base::Unretained(this)));
-}
-
 }  // namespace memory

chrome/browser/memory/swap_thrashing_monitor.h

@@ -23,9 +23,13 @@
 #ifndef CHROME_BROWSER_MEMORY_SWAP_THRASHING_MONITOR_H_
 #define CHROME_BROWSER_MEMORY_SWAP_THRASHING_MONITOR_H_
 
+#include <memory>
+
 #include "base/base_export.h"
 #include "base/feature_list.h"
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
+#include "base/task/post_task.h"
 #include "base/timer/timer.h"
 #include "build/build_config.h"
 #include "chrome/browser/memory/swap_thrashing_monitor_delegate.h"
@@ -71,8 +75,18 @@ class SwapThrashingMonitor {
   void StartObserving();
 
  private:
-  // The delegate responsible for measuring the swap-thrashing activity.
-  std::unique_ptr<SwapThrashingMonitorDelegate> delegate_;
+  void RecordSwapThrashingLevel(SwapThrashingLevel swap_thrashing_level);
+
+  // The task runner used to run blocking operations.
+  scoped_refptr<base::SequencedTaskRunner> blocking_task_runner_ =
+      base::CreateSequencedTaskRunnerWithTraits(
+          {base::MayBlock(), base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN});
+
+  // The delegate responsible for measuring the swap-thrashing activity. This
+  // task runner is expected to be used and destroyed on
+  // |blocking_task_runner_|.
+  std::unique_ptr<SwapThrashingMonitorDelegate, base::OnTaskRunnerDeleter>
+      delegate_;
 
   SwapThrashingLevel current_swap_thrashing_level_;
 
@@ -80,6 +94,8 @@ class SwapThrashingMonitor {
 
   SEQUENCE_CHECKER(sequence_checker_);
 
+  base::WeakPtrFactory<SwapThrashingMonitor> weak_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(SwapThrashingMonitor);
 };
 

chrome/browser/memory/swap_thrashing_monitor_delegate_win.cc

@@ -7,9 +7,11 @@
 #include <windows.h>
 #include <winternl.h>
 
+#include <memory>
 #include <vector>
 
 #include "base/files/file_path.h"
+#include "base/threading/scoped_blocking_call.h"
 #include "base/win/win_util.h"
 #include "chrome/common/chrome_constants.h"
 
@@ -87,24 +89,28 @@ base::Optional<int64_t> GetHardFaultCountForChromeProcesses() {
   //       and threads running on the system. The initial guess suffices for
   //       ~100s of processes and ~1000s of threads.
   std::vector<uint8_t> buffer(32 * 1024);
-  for (size_t tries = 0; tries < 3; ++tries) {
-    ULONG return_length = 0;
-    const NTSTATUS status = query_system_information_ptr(
-        SystemProcessInformation, buffer.data(),
-        static_cast<ULONG>(buffer.size()), &return_length);
 
-    if (status == STATUS_SUCCESS)
-      break;
-
-    if (status == STATUS_INFO_LENGTH_MISMATCH ||
-        status == STATUS_BUFFER_TOO_SMALL) {
-      // Insufficient buffer. Grow to the returned |return_length| plus 10%
-      // extra to avoid frequent reallocations and try again.
-      DCHECK_GT(return_length, buffer.size());
-      buffer.resize(static_cast<ULONG>(return_length * 1.1));
-    } else {
-      // An error other than the two above.
-      return base::nullopt;
+  base::ScopedBlockingCall scoped_blocking_call(base::BlockingType::MAY_BLOCK);
+  {
+    for (size_t tries = 0; tries < 3; ++tries) {
+      ULONG return_length = 0;
+      const NTSTATUS status = query_system_information_ptr(
+          SystemProcessInformation, buffer.data(),
+          static_cast<ULONG>(buffer.size()), &return_length);
+
+      if (status == STATUS_SUCCESS)
+        break;
+
+      if (status == STATUS_INFO_LENGTH_MISMATCH ||
+          status == STATUS_BUFFER_TOO_SMALL) {
+        // Insufficient buffer. Grow to the returned |return_length| plus 10%
+        // extra to avoid frequent reallocations and try again.
+        DCHECK_GT(return_length, buffer.size());
+        buffer.resize(static_cast<ULONG>(return_length * 1.1));
+      } else {
+        // An error other than the two above.
+        return base::nullopt;
+      }
     }
   }
 
@@ -135,15 +141,25 @@ const size_t SwapThrashingMonitorDelegateWin::HardFaultDeltasWindow::
     kHardFaultDeltasWindowSize = 12;
 
 SwapThrashingMonitorDelegateWin::SwapThrashingMonitorDelegateWin()
-    : hard_fault_deltas_window_(std::make_unique<HardFaultDeltasWindow>()) {}
+    : hard_fault_deltas_window_(std::make_unique<HardFaultDeltasWindow>()) {
+  // This object can be created on a sequence different than the one where it is
+  // used. In practice this object is created on the UI thread and it is then
+  // used and destroyed on a MayBlock sequence.
+  DETACH_FROM_SEQUENCE(sequence_checker_);
+}
 
-SwapThrashingMonitorDelegateWin::~SwapThrashingMonitorDelegateWin() {}
+SwapThrashingMonitorDelegateWin::~SwapThrashingMonitorDelegateWin() = default;
 
 SwapThrashingMonitorDelegateWin::HardFaultDeltasWindow::HardFaultDeltasWindow()
-    : latest_hard_fault_count_(), observation_above_threshold_count_(0U) {}
+    : latest_hard_fault_count_(), observation_above_threshold_count_(0U) {
+  // This object will be created as the same time as the
+  // SwapThrashingMonitorDelegateWin object that owns it, after that it might be
+  // used on a different sequence.
+  DETACH_FROM_SEQUENCE(sequence_checker_);
+}
 
 SwapThrashingMonitorDelegateWin::HardFaultDeltasWindow::
-    ~HardFaultDeltasWindow() {}
+    ~HardFaultDeltasWindow() = default;
 
 void SwapThrashingMonitorDelegateWin::HardFaultDeltasWindow::OnObservation(
     uint64_t hard_fault_count) {

chrome/browser/memory/swap_thrashing_monitor_delegate_win.h

@@ -18,6 +18,9 @@
 
 namespace memory {
 
+// This delegate can be created from any sequence but it has to be used on a
+// MayBlock sequence as the work done to compute the thrashing level might be
+// blocking.
 class SwapThrashingMonitorDelegateWin : public SwapThrashingMonitorDelegate {
  public:
   SwapThrashingMonitorDelegateWin();