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}
diff --git a/AUTHORS b/AUTHORS
index a68e49b..30f3f22 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -1328,6 +1328,7 @@
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
index 7018d4c..eb9c54f 100644
--- 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,6 +213,7 @@
}
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
@@ -219,28 +221,24 @@
// 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.
@@ -248,6 +246,9 @@
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
index 72f2737..a249519 100644
--- a/base/threading/platform_thread.h
+++ b/base/threading/platform_thread.h
@@ -85,6 +85,7 @@
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
index 8a27505..edd89c3b 100644
--- 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
index 4720d7c..dfb8bdec 100644
--- a/base/threading/scoped_thread_priority.h
+++ b/base/threading/scoped_thread_priority.h
@@ -63,6 +63,20 @@
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
index 4bc8169..6973349 100644
--- 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 @@
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 @@
} // 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
index 4f4c0f8..6eb6f9f 100644
--- 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 @@
// 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;
