Commit 8a783b30 authored by Francois Doray's avatar Francois Doray Committed by Commit Bot

[Base] Use background mode for ThreadPriority::BACKGROUND threads (behind feature) (reland).

The previous attempt to land this CL was reverted because an early call
to PlatformThread::SetCurrentThreadPriority() in headless caused the
FeatureList to be accessed before it was initialized. This CL avoids
accessing an uninitialized FeatureList if the target priority is not
BACKGROUND.

Diff between ps 1 and 5 shows changes made since revert.

Previous attempts to land:
  https://chromium-review.googlesource.com/1171482
  https://chromium-review.googlesource.com/1185529
  https://chromium-review.googlesource.com/1190842

NOTE: If this causes new test failures, we'll add the value returned
by GetThreadPriority() to the switch/case of allowed values.

This CL adds a feature to use THREAD_MODE_BACKGROUND_BEGIN instead of
THREAD_PRIORITY_LOWEST for ThreadPriority::BACKGROUND threads. This
lowers the disk and network I/O priority of the thread in addition to
the CPU scheduling priority. MSDN recommends using this setting for
threads that perform background work.
https://docs.microsoft.com/en-us/windows/desktop/api/processthreadsapi/nf-processthreadsapi-setthreadpriority

Bug: 872820
Change-Id: I2c9f17390eaaba2f8f454faa29836bbc37e3d47c
Reviewed-on: https://chromium-review.googlesource.com/1195677
Commit-Queue: François Doray <fdoray@chromium.org>
Reviewed-by: default avatarGabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587349}
parent 0c1c9415
...@@ -939,6 +939,7 @@ jumbo_component("base") { ...@@ -939,6 +939,7 @@ jumbo_component("base") {
"threading/platform_thread_linux.cc", "threading/platform_thread_linux.cc",
"threading/platform_thread_mac.mm", "threading/platform_thread_mac.mm",
"threading/platform_thread_win.cc", "threading/platform_thread_win.cc",
"threading/platform_thread_win.h",
"threading/post_task_and_reply_impl.cc", "threading/post_task_and_reply_impl.cc",
"threading/post_task_and_reply_impl.h", "threading/post_task_and_reply_impl.h",
"threading/scoped_blocking_call.cc", "threading/scoped_blocking_call.cc",
......
...@@ -7,6 +7,7 @@ ...@@ -7,6 +7,7 @@
#include "base/compiler_specific.h" #include "base/compiler_specific.h"
#include "base/macros.h" #include "base/macros.h"
#include "base/synchronization/waitable_event.h" #include "base/synchronization/waitable_event.h"
#include "base/test/scoped_feature_list.h"
#include "base/threading/platform_thread.h" #include "base/threading/platform_thread.h"
#include "build/build_config.h" #include "build/build_config.h"
#include "testing/gtest/include/gtest/gtest.h" #include "testing/gtest/include/gtest/gtest.h"
...@@ -15,6 +16,7 @@ ...@@ -15,6 +16,7 @@
#include "base/threading/platform_thread_internal_posix.h" #include "base/threading/platform_thread_internal_posix.h"
#elif defined(OS_WIN) #elif defined(OS_WIN)
#include <windows.h> #include <windows.h>
#include "base/threading/platform_thread_win.h"
#endif #endif
namespace base { namespace base {
...@@ -288,16 +290,7 @@ class ThreadPriorityTestThread : public FunctionTestThread { ...@@ -288,16 +290,7 @@ class ThreadPriorityTestThread : public FunctionTestThread {
DISALLOW_COPY_AND_ASSIGN(ThreadPriorityTestThread); DISALLOW_COPY_AND_ASSIGN(ThreadPriorityTestThread);
}; };
} // namespace void TestSetCurrentThreadPriority() {
// Test changing a created thread's priority (which has different semantics on
// some platforms).
#if defined(OS_FUCHSIA)
// TODO(crbug.com/851759): Thread priorities are not implemented in Fuchsia.
#define MAYBE_SetCurrentThreadPriority DISABLED_SetCurrentThreadPriority
#else
#define MAYBE_SetCurrentThreadPriority SetCurrentThreadPriority
#endif
TEST(PlatformThreadTest, MAYBE_SetCurrentThreadPriority) {
ThreadPriorityTestThread thread; ThreadPriorityTestThread thread;
PlatformThreadHandle handle; PlatformThreadHandle handle;
...@@ -311,6 +304,30 @@ TEST(PlatformThreadTest, MAYBE_SetCurrentThreadPriority) { ...@@ -311,6 +304,30 @@ TEST(PlatformThreadTest, MAYBE_SetCurrentThreadPriority) {
ASSERT_FALSE(thread.IsRunning()); ASSERT_FALSE(thread.IsRunning());
} }
} // namespace
// Test changing a created thread's priority.
#if defined(OS_FUCHSIA)
// TODO(crbug.com/851759): Thread priorities are not implemented in Fuchsia.
#define MAYBE_SetCurrentThreadPriority DISABLED_SetCurrentThreadPriority
#else
#define MAYBE_SetCurrentThreadPriority SetCurrentThreadPriority
#endif
TEST(PlatformThreadTest, MAYBE_SetCurrentThreadPriority) {
TestSetCurrentThreadPriority();
}
#if defined(OS_WIN)
// Test changing a created thread's priority, with the
// kWindowsThreadModeBackground feature enabled.
TEST(PlatformThreadTest, SetCurrentThreadPriorityWithThreadModeBackground) {
test::ScopedFeatureList scoped_feature_list;
scoped_feature_list.InitAndEnableFeature(
features::kWindowsThreadModeBackground);
TestSetCurrentThreadPriority();
}
#endif // defined(OS_WIN)
// This tests internal PlatformThread APIs used under some POSIX platforms, // This tests internal PlatformThread APIs used under some POSIX platforms,
// with the exception of Mac OS X, iOS and Fuchsia. // with the exception of Mac OS X, iOS and Fuchsia.
#if defined(OS_POSIX) && !defined(OS_MACOSX) && !defined(OS_IOS) && \ #if defined(OS_POSIX) && !defined(OS_MACOSX) && !defined(OS_IOS) && \
......
...@@ -2,7 +2,7 @@ ...@@ -2,7 +2,7 @@
// Use of this source code is governed by a BSD-style license that can be // Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file. // found in the LICENSE file.
#include "base/threading/platform_thread.h" #include "base/threading/platform_thread_win.h"
#include <stddef.h> #include <stddef.h>
...@@ -15,6 +15,7 @@ ...@@ -15,6 +15,7 @@
#include "base/threading/thread_id_name_manager.h" #include "base/threading/thread_id_name_manager.h"
#include "base/threading/thread_restrictions.h" #include "base/threading/thread_restrictions.h"
#include "base/win/scoped_handle.h" #include "base/win/scoped_handle.h"
#include "base/win/windows_version.h"
#include <windows.h> #include <windows.h>
...@@ -22,6 +23,14 @@ namespace base { ...@@ -22,6 +23,14 @@ namespace base {
namespace { namespace {
// The value returned by ::GetThreadPriority() after background thread mode is
// enabled on Windows 7.
constexpr int kWin7BackgroundThreadModePriority = 4;
// The value returned by ::GetThreadPriority() after background thread mode is
// enabled on Windows 8+.
constexpr int kWin8AboveBackgroundThreadModePriority = -4;
// The information on how to set the thread name comes from // The information on how to set the thread name comes from
// a MSDN article: http://msdn2.microsoft.com/en-us/library/xcb2z8hs.aspx // a MSDN article: http://msdn2.microsoft.com/en-us/library/xcb2z8hs.aspx
const DWORD kVCThreadNameException = 0x406D1388; const DWORD kVCThreadNameException = 0x406D1388;
...@@ -144,6 +153,11 @@ bool CreateThreadInternal(size_t stack_size, ...@@ -144,6 +153,11 @@ bool CreateThreadInternal(size_t stack_size,
} // namespace } // namespace
namespace features {
const Feature kWindowsThreadModeBackground{"WindowsThreadModeBackground",
FEATURE_DISABLED_BY_DEFAULT};
} // namespace features
// static // static
PlatformThreadId PlatformThread::CurrentId() { PlatformThreadId PlatformThread::CurrentId() {
return ::GetCurrentThreadId(); return ::GetCurrentThreadId();
...@@ -263,10 +277,32 @@ bool PlatformThread::CanIncreaseCurrentThreadPriority() { ...@@ -263,10 +277,32 @@ bool PlatformThread::CanIncreaseCurrentThreadPriority() {
// static // static
void PlatformThread::SetCurrentThreadPriority(ThreadPriority priority) { void PlatformThread::SetCurrentThreadPriority(ThreadPriority priority) {
// A DCHECK is triggered on FeatureList initialization if the state of a
// feature has been checked before. We only want to trigger that DCHECK if the
// priority has been set to BACKGROUND before, so we are careful not to access
// the state of the feature needlessly. We don't DCHECK here because it is ok
// if the FeatureList is never initialized in the process (e.g. in tests).
//
// TODO(fdoray): Remove experiment code. https://crbug.com/872820
const bool use_thread_mode_background =
(priority == ThreadPriority::BACKGROUND
? FeatureList::IsEnabled(features::kWindowsThreadModeBackground)
: (FeatureList::GetInstance() &&
FeatureList::IsEnabled(features::kWindowsThreadModeBackground)));
if (use_thread_mode_background && priority != ThreadPriority::BACKGROUND) {
// Exit background mode if the new priority is not BACKGROUND. This is a
// no-op if not in background mode.
::SetThreadPriority(PlatformThread::CurrentHandle().platform_handle(),
THREAD_MODE_BACKGROUND_END);
}
int desired_priority = THREAD_PRIORITY_ERROR_RETURN; int desired_priority = THREAD_PRIORITY_ERROR_RETURN;
switch (priority) { switch (priority) {
case ThreadPriority::BACKGROUND: case ThreadPriority::BACKGROUND:
desired_priority = THREAD_PRIORITY_LOWEST; desired_priority = use_thread_mode_background
? THREAD_MODE_BACKGROUND_BEGIN
: THREAD_PRIORITY_LOWEST;
break; break;
case ThreadPriority::NORMAL: case ThreadPriority::NORMAL:
desired_priority = THREAD_PRIORITY_NORMAL; desired_priority = THREAD_PRIORITY_NORMAL;
...@@ -290,13 +326,23 @@ void PlatformThread::SetCurrentThreadPriority(ThreadPriority priority) { ...@@ -290,13 +326,23 @@ void PlatformThread::SetCurrentThreadPriority(ThreadPriority priority) {
desired_priority); desired_priority);
DPLOG_IF(ERROR, !success) << "Failed to set thread priority to " DPLOG_IF(ERROR, !success) << "Failed to set thread priority to "
<< desired_priority; << desired_priority;
// Sanity check that GetCurrentThreadPriority() is consistent with
// SetCurrentThreadPriority().
DCHECK_EQ(GetCurrentThreadPriority(), priority);
} }
// static // static
ThreadPriority PlatformThread::GetCurrentThreadPriority() { ThreadPriority PlatformThread::GetCurrentThreadPriority() {
int priority = const int priority =
::GetThreadPriority(PlatformThread::CurrentHandle().platform_handle()); ::GetThreadPriority(PlatformThread::CurrentHandle().platform_handle());
switch (priority) { switch (priority) {
case THREAD_PRIORITY_IDLE:
case kWin7BackgroundThreadModePriority:
DCHECK_EQ(win::GetVersion(), win::VERSION_WIN7);
FALLTHROUGH;
case kWin8AboveBackgroundThreadModePriority:
case THREAD_PRIORITY_LOWEST: case THREAD_PRIORITY_LOWEST:
return ThreadPriority::BACKGROUND; return ThreadPriority::BACKGROUND;
case THREAD_PRIORITY_NORMAL: case THREAD_PRIORITY_NORMAL:
...@@ -307,11 +353,10 @@ ThreadPriority PlatformThread::GetCurrentThreadPriority() { ...@@ -307,11 +353,10 @@ ThreadPriority PlatformThread::GetCurrentThreadPriority() {
return ThreadPriority::REALTIME_AUDIO; return ThreadPriority::REALTIME_AUDIO;
case THREAD_PRIORITY_ERROR_RETURN: case THREAD_PRIORITY_ERROR_RETURN:
DPCHECK(false) << "GetThreadPriority error"; DPCHECK(false) << "GetThreadPriority error";
FALLTHROUGH;
default:
NOTREACHED() << "Unexpected priority: " << priority;
return ThreadPriority::NORMAL;
} }
NOTREACHED() << "GetCurrentThreadPriority returned " << priority << ".";
return ThreadPriority::NORMAL;
} }
} // namespace base } // namespace base
// Copyright 2018 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 BASE_THREADING_PLATFORM_THREAD_WIN_H_
#define BASE_THREADING_PLATFORM_THREAD_WIN_H_
#include "base/threading/platform_thread.h"
#include "base/base_export.h"
#include "base/feature_list.h"
namespace base {
namespace features {
// Use THREAD_MODE_BACKGROUND_BEGIN instead of THREAD_PRIORITY_LOWEST for
// ThreadPriority::BACKGROUND threads. This lowers the disk and network I/O
// priority of the thread in addition to the CPU scheduling priority. MSDN
// recommends using this setting for threads that perform background work.
// https://docs.microsoft.com/en-us/windows/desktop/api/processthreadsapi/nf-processthreadsapi-setthreadpriority
BASE_EXPORT extern const Feature kWindowsThreadModeBackground;
} // namespace features
} // namespace base
#endif // BASE_THREADING_PLATFORM_THREAD_WIN_H_
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment