Create base::ScopedBoostPriority

This patch provides with a base::ScopedBoostPriority class for temporarily boosting thread priority. The original thread priority will be set back again when the object is destructed. This fixes a bug where the priority was boosted even when the platform was unable to undo the operation. This might as a side-effect keep threads at NORMAL priority that were previously unintentionally permanently boosted to DISPLAY priority. Fixed: 1335489 Change-Id: I6a58f5b4c5a7dace86bd7788f6ceca7ecb9baeb8 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3699224 Reviewed-by: Gabriel Charette <gab@chromium.org> Commit-Queue: Zhibo Wang <zhibo1.wang@intel.com> Cr-Commit-Position: refs/heads/main@{#1013320}
marcosholgado · Jun 12, 2022 · 9906407 · 9906407
1 parent 664df7a
commit 9906407
Show file tree

Hide file tree

Showing 7 changed files with 111 additions and 36 deletions.
diff --git a/AUTHORS b/AUTHORS
@@ -1328,6 +1328,7 @@ Zheng Xu <zxu@kobo.com>
 Zhengkun Li <zhengkli@amazon.com>
 Zhenyu Liang <zhenyu.liang@intel.com>
 Zhenyu Shan <zhenyu.shan@intel.com>
+Zhibo Wang <zhibo1.wang@intel.com>
 Zhifei Fang <facetothefate@gmail.com>
 Zhiyuan Ye <zhiyuanye@tencent.com>
 Zhuoyu Qian <zhuoyu.qian@samsung.com>

diff --git a/base/files/important_file_writer.cc b/base/files/important_file_writer.cc
@@ -31,6 +31,7 @@
 #include "base/task/task_runner.h"
 #include "base/task/task_runner_util.h"
 #include "base/threading/platform_thread.h"
+#include "base/threading/scoped_thread_priority.h"
 #include "base/threading/sequenced_task_runner_handle.h"
 #include "base/threading/thread.h"
 #include "base/time/time.h"
@@ -212,42 +213,42 @@ bool ImportantFileWriter::WriteFileAtomicallyImpl(const FilePath& path,
   }
 
   File::Error replace_file_error = File::FILE_OK;
+  bool result;
 
   // The file must be closed for ReplaceFile to do its job, which opens up a
   // race with other software that may open the temp file (e.g., an A/V scanner
   // doing its job without oplocks). Boost a background thread's priority on
   // Windows and close as late as possible to improve the chances that the other
   // software will lose the race.
 #if BUILDFLAG(IS_WIN)
-  const auto previous_priority = PlatformThread::GetCurrentThreadPriority();
-  const bool reset_priority = previous_priority <= ThreadPriority::NORMAL;
-  if (reset_priority)
-    PlatformThread::SetCurrentThreadPriority(ThreadPriority::DISPLAY);
-#endif  // BUILDFLAG(IS_WIN)
-  tmp_file.Close();
-  bool result = ReplaceFile(tmp_file_path, path, &replace_file_error);
-#if BUILDFLAG(IS_WIN)
-  // Save and restore the last error code so that it's not polluted by the
-  // thread priority change.
-  auto last_error = ::GetLastError();
+  DWORD last_error;
   int retry_count = 0;
-  for (/**/; !result && retry_count < kReplaceRetries; ++retry_count) {
-    // The race condition between closing the temporary file and moving it gets
-    // hit on a regular basis on some systems (https://crbug.com/1099284), so
-    // we retry a few times before giving up.
-    PlatformThread::Sleep(kReplacePauseInterval);
+  {
+    ScopedBoostPriority scoped_boost_priority(ThreadPriority::DISPLAY);
+    tmp_file.Close();
     result = ReplaceFile(tmp_file_path, path, &replace_file_error);
+    // Save and restore the last error code so that it's not polluted by the
+    // thread priority change.
     last_error = ::GetLastError();
+    for (/**/; !result && retry_count < kReplaceRetries; ++retry_count) {
+      // The race condition between closing the temporary file and moving it
+      // gets hit on a regular basis on some systems
+      // (https://crbug.com/1099284), so we retry a few times before giving up.
+      PlatformThread::Sleep(kReplacePauseInterval);
+      result = ReplaceFile(tmp_file_path, path, &replace_file_error);
+      last_error = ::GetLastError();
+    }
   }
-  if (reset_priority)
-    PlatformThread::SetCurrentThreadPriority(previous_priority);
 
   // Log how many times we had to retry the ReplaceFile operation before it
   // succeeded. If we never succeeded then return a special value.
   if (!result)
     retry_count = kReplaceRetryFailure;
   UmaHistogramExactLinear("ImportantFile.FileReplaceRetryCount", retry_count,
                           kReplaceRetryFailure);
+#else
+  tmp_file.Close();
+  result = ReplaceFile(tmp_file_path, path, &replace_file_error);
 #endif  // BUILDFLAG(IS_WIN)
 
   if (!result) {

diff --git a/base/threading/platform_thread.h b/base/threading/platform_thread.h
@@ -85,6 +85,7 @@ enum class ThreadPriority : int {
   DISPLAY,
   // Suitable for low-latency, glitch-resistant audio.
   REALTIME_AUDIO,
+  kMaxValue = REALTIME_AUDIO,
 };
 
 // A namespace for low-level thread functions.

diff --git a/base/threading/scoped_thread_priority.cc b/base/threading/scoped_thread_priority.cc
@@ -10,6 +10,27 @@
 #include "build/build_config.h"
 
 namespace base {
+
+ScopedBoostPriority::ScopedBoostPriority(ThreadPriority target_priority) {
+  DCHECK_LT(target_priority, ThreadPriority::REALTIME_AUDIO);
+  const ThreadPriority original_priority =
+      PlatformThread::GetCurrentThreadPriority();
+  const bool should_boost = original_priority < target_priority &&
+                            PlatformThread::CanChangeThreadPriority(
+                                original_priority, target_priority) &&
+                            PlatformThread::CanChangeThreadPriority(
+                                target_priority, original_priority);
+  if (should_boost) {
+    original_priority_.emplace(original_priority);
+    PlatformThread::SetCurrentThreadPriority(target_priority);
+  }
+}
+
+ScopedBoostPriority::~ScopedBoostPriority() {
+  if (original_priority_.has_value())
+    PlatformThread::SetCurrentThreadPriority(original_priority_.value());
+}
+
 namespace internal {
 
 ScopedMayLoadLibraryAtBackgroundPriority::

diff --git a/base/threading/scoped_thread_priority.h b/base/threading/scoped_thread_priority.h
@@ -63,6 +63,20 @@ enum class ThreadPriority : int;
       INTERNAL_SCOPED_THREAD_PRIORITY_APPEND_LINE(                  \
           scoped_may_load_library_at_background_priority)(FROM_HERE, nullptr);
 
+// Boosts the current thread's priority to match the priority of threads of
+// |target_thread_type| in this scope.
+class BASE_EXPORT ScopedBoostPriority {
+ public:
+  explicit ScopedBoostPriority(ThreadPriority target_priority);
+  ~ScopedBoostPriority();
+
+  ScopedBoostPriority(const ScopedBoostPriority&) = delete;
+  ScopedBoostPriority& operator=(const ScopedBoostPriority&) = delete;
+
+ private:
+  absl::optional<ThreadPriority> original_priority_;
+};
+
 namespace internal {
 
 class BASE_EXPORT ScopedMayLoadLibraryAtBackgroundPriority {

diff --git a/base/threading/scoped_thread_priority_unittest.cc b/base/threading/scoped_thread_priority_unittest.cc
@@ -5,6 +5,7 @@
 #include "base/threading/scoped_thread_priority.h"
 
 #include "base/threading/platform_thread.h"
+#include "base/threading/thread.h"
 #include "build/build_config.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -21,6 +22,15 @@ namespace {
     ADD_FAILURE() << "This test cannot run multiple times in the same " \
                      "process.";
 
+static ThreadPriority kAllThreadTypes[] = {
+    ThreadPriority::REALTIME_AUDIO, ThreadPriority::DISPLAY,
+    ThreadPriority::NORMAL, ThreadPriority::BACKGROUND};
+
+static_assert(static_cast<int>(ThreadPriority::BACKGROUND) == 0,
+              "kBackground isn't lowest");
+static_assert(ThreadPriority::REALTIME_AUDIO == ThreadPriority::kMaxValue,
+              "kRealtimeAudio isn't highest");
+
 class ScopedThreadPriorityTest : public testing::Test {
  protected:
   void SetUp() override {
@@ -46,6 +56,41 @@ void FunctionThatBoostsPriorityOnEveryInvoke() {
 
 }  // namespace
 
+TEST_F(ScopedThreadPriorityTest, BasicTest) {
+  for (auto from : kAllThreadTypes) {
+    if (!PlatformThread::CanChangeThreadPriority(ThreadPriority::NORMAL, from))
+      continue;
+    for (auto to : kAllThreadTypes) {
+      // ThreadType::kRealtimeAudio is not a valid |target_thread_type| for
+      // ScopedBoostPriority.
+      if (to == ThreadPriority::REALTIME_AUDIO)
+        continue;
+      Thread::Options options;
+      options.priority = from;
+      Thread thread("ScopedThreadPriorityTest");
+      thread.StartWithOptions(std::move(options));
+      thread.WaitUntilThreadStarted();
+      thread.task_runner()->PostTask(
+          FROM_HERE,
+          BindOnce(
+              [](ThreadPriority from, ThreadPriority to) {
+                EXPECT_EQ(PlatformThread::GetCurrentThreadPriority(), from);
+                {
+                  ScopedBoostPriority scoped_boost_priority(to);
+                  bool will_boost_priority =
+                      from < to &&
+                      PlatformThread::CanChangeThreadPriority(from, to) &&
+                      PlatformThread::CanChangeThreadPriority(to, from);
+                  EXPECT_EQ(PlatformThread::GetCurrentThreadPriority(),
+                            will_boost_priority ? to : from);
+                }
+                EXPECT_EQ(PlatformThread::GetCurrentThreadPriority(), from);
+              },
+              from, to));
+    }
+  }
+}
+
 TEST_F(ScopedThreadPriorityTest, WithoutPriorityBoost) {
   ASSERT_RUNS_ONCE();
 

diff --git a/components/startup_metric_utils/browser/startup_metric_utils.cc b/components/startup_metric_utils/browser/startup_metric_utils.cc
@@ -21,6 +21,7 @@
 #include "base/strings/strcat.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/threading/platform_thread.h"
+#include "base/threading/scoped_thread_priority.h"
 #include "base/time/time.h"
 #include "base/trace_event/trace_event.h"
 #include "build/build_config.h"
@@ -300,35 +301,26 @@ void RecordHardFaultHistogram() {
 // base::TimeTicks::Now() at play, but in practice it is pretty much instant
 // compared to multi-seconds startup timings.
 base::TimeTicks StartupTimeToTimeTicks(base::Time time) {
-// First get a base which represents the same point in time in both units.
-// Bump the priority of this thread while doing this as the wall clock time it
-// takes to resolve these two calls affects the precision of this method and
-// bumping the priority reduces the likelihood of a context switch interfering
-// with this computation.
+  // First get a base which represents the same point in time in both units.
+  // Bump the priority of this thread while doing this as the wall clock time it
+  // takes to resolve these two calls affects the precision of this method and
+  // bumping the priority reduces the likelihood of a context switch interfering
+  // with this computation.
+  absl::optional<base::ScopedBoostPriority> scoped_boost_priority;
 
 // Enabling this logic on OS X causes a significant performance regression.
 // https://crbug.com/601270
 #if !BUILDFLAG(IS_APPLE)
   static bool statics_initialized = false;
-
-  base::ThreadPriority previous_priority = base::ThreadPriority::NORMAL;
   if (!statics_initialized) {
-    previous_priority = base::PlatformThread::GetCurrentThreadPriority();
-    base::PlatformThread::SetCurrentThreadPriority(
-        base::ThreadPriority::DISPLAY);
+    statics_initialized = true;
+    scoped_boost_priority.emplace(base::ThreadPriority::DISPLAY);
   }
-#endif
+#endif  // !BUILDFLAG(IS_APPLE)
 
   static const base::Time time_base = base::Time::Now();
   static const base::TimeTicks trace_ticks_base = base::TimeTicks::Now();
 
-#if !BUILDFLAG(IS_APPLE)
-  if (!statics_initialized) {
-    base::PlatformThread::SetCurrentThreadPriority(previous_priority);
-  }
-  statics_initialized = true;
-#endif
-
   // Then use the TimeDelta common ground between the two units to make the
   // conversion.
   const base::TimeDelta delta_since_base = time_base - time;