Commit 726b2b57 authored by Vadym Doroshenko's avatar Vadym Doroshenko Committed by Commit Bot

Revert "Base: Ensure that SetCurrentThreadPriority(BACKGROUND) sets CPU priority on Windows."

This reverts commit 5cb9cc29.

Reason for revert: test PlatformThreadWinTest.SetBackgroundThreadModeFailsInIdlePriorityProcess which was added on this CL fails on some bots, 
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Win7%20%2832%29%20Tests/40510

Original change's description:
> Base: Ensure that SetCurrentThreadPriority(BACKGROUND) sets CPU priority on Windows.
> 
> Previously, SetCurrentThreadPriority(BACKGROUND) did not affect CPU
> priority on Windows in a IDLE process when the
> "WindowsThreadModeBackground" feature was enabled. It only affected I/O
> and memory priorities.
> 
> With this CL, a second call to ::SetThreadPriority() is made to
> ensure that CPU priority is affected.
> 
> Bug: 901483
> Change-Id: I22640c8fa56eeec489fbf0fe516359b828e39541
> Reviewed-on: https://chromium-review.googlesource.com/c/1318438
> Commit-Queue: François Doray <fdoray@chromium.org>
> Reviewed-by: Gabriel Charette <gab@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#606200}

TBR=gab@chromium.org,fdoray@chromium.org

Change-Id: I6ea74d6d1fa14a7b668292e7c7ef75c5ace643e6
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 901483
Reviewed-on: https://chromium-review.googlesource.com/c/1325990Reviewed-by: default avatarVadym Doroshenko <dvadym@chromium.org>
Commit-Queue: Vadym Doroshenko <dvadym@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606429}
parent 0d1087c7
...@@ -2497,7 +2497,6 @@ test("base_unittests") { ...@@ -2497,7 +2497,6 @@ test("base_unittests") {
"test/trace_event_analyzer_unittest.cc", "test/trace_event_analyzer_unittest.cc",
"thread_annotations_unittest.cc", "thread_annotations_unittest.cc",
"threading/platform_thread_unittest.cc", "threading/platform_thread_unittest.cc",
"threading/platform_thread_win_unittest.cc",
"threading/post_task_and_reply_impl_unittest.cc", "threading/post_task_and_reply_impl_unittest.cc",
"threading/scoped_blocking_call_unittest.cc", "threading/scoped_blocking_call_unittest.cc",
"threading/sequence_bound_unittest.cc", "threading/sequence_bound_unittest.cc",
......
...@@ -295,21 +295,6 @@ TEST(PlatformThreadTest, SetCurrentThreadPriorityWithThreadModeBackground) { ...@@ -295,21 +295,6 @@ TEST(PlatformThreadTest, SetCurrentThreadPriorityWithThreadModeBackground) {
features::kWindowsThreadModeBackground); features::kWindowsThreadModeBackground);
TestSetCurrentThreadPriority(); TestSetCurrentThreadPriority();
} }
// Test changing a created thread's priority, with the
// kWindowsThreadModeBackground feature enabled, in a IDLE_PRIORITY_CLASS
// process (regression test for https://crbug.com/901483).
TEST(PlatformThreadTest,
SetCurrentThreadPriorityWithThreadModeBackgroundIdleProcess) {
::SetPriorityClass(Process::Current().Handle(), IDLE_PRIORITY_CLASS);
test::ScopedFeatureList scoped_feature_list;
scoped_feature_list.InitAndEnableFeature(
features::kWindowsThreadModeBackground);
TestSetCurrentThreadPriority();
::SetPriorityClass(Process::Current().Handle(), NORMAL_PRIORITY_CLASS);
}
#endif // defined(OS_WIN) #endif // defined(OS_WIN)
// Ideally PlatformThread::CanIncreaseThreadPriority() would be true on all // Ideally PlatformThread::CanIncreaseThreadPriority() would be true on all
......
...@@ -158,31 +158,6 @@ const Feature kWindowsThreadModeBackground{"WindowsThreadModeBackground", ...@@ -158,31 +158,6 @@ const Feature kWindowsThreadModeBackground{"WindowsThreadModeBackground",
FEATURE_DISABLED_BY_DEFAULT}; FEATURE_DISABLED_BY_DEFAULT};
} // namespace features } // namespace features
namespace internal {
void AssertMemoryPriority(HANDLE thread, int memory_priority) {
#if DCHECK_IS_ON()
static const auto get_thread_information_fn =
reinterpret_cast<decltype(&::GetThreadInformation)>(::GetProcAddress(
::GetModuleHandle(L"Kernel32.dll"), "GetThreadInformation"));
if (!get_thread_information_fn) {
DCHECK_EQ(win::GetVersion(), win::VERSION_WIN7);
return;
}
MEMORY_PRIORITY_INFORMATION memory_priority_information = {};
DCHECK(get_thread_information_fn(thread, ::ThreadMemoryPriority,
&memory_priority_information,
sizeof(memory_priority_information)));
DCHECK_EQ(memory_priority,
static_cast<int>(memory_priority_information.MemoryPriority));
#endif
}
} // namespace internal
// static // static
PlatformThreadId PlatformThread::CurrentId() { PlatformThreadId PlatformThread::CurrentId() {
return ::GetCurrentThreadId(); return ::GetCurrentThreadId();
...@@ -315,14 +290,11 @@ void PlatformThread::SetCurrentThreadPriority(ThreadPriority priority) { ...@@ -315,14 +290,11 @@ void PlatformThread::SetCurrentThreadPriority(ThreadPriority priority) {
: (FeatureList::GetInstance() && : (FeatureList::GetInstance() &&
FeatureList::IsEnabled(features::kWindowsThreadModeBackground))); FeatureList::IsEnabled(features::kWindowsThreadModeBackground)));
PlatformThreadHandle::Handle thread_handle =
PlatformThread::CurrentHandle().platform_handle();
if (use_thread_mode_background && priority != ThreadPriority::BACKGROUND) { if (use_thread_mode_background && priority != ThreadPriority::BACKGROUND) {
// Exit background mode if the new priority is not BACKGROUND. This is a // Exit background mode if the new priority is not BACKGROUND. This is a
// no-op if not in background mode. // no-op if not in background mode.
::SetThreadPriority(thread_handle, THREAD_MODE_BACKGROUND_END); ::SetThreadPriority(PlatformThread::CurrentHandle().platform_handle(),
internal::AssertMemoryPriority(thread_handle, MEMORY_PRIORITY_NORMAL); THREAD_MODE_BACKGROUND_END);
} }
int desired_priority = THREAD_PRIORITY_ERROR_RETURN; int desired_priority = THREAD_PRIORITY_ERROR_RETURN;
...@@ -350,24 +322,13 @@ void PlatformThread::SetCurrentThreadPriority(ThreadPriority priority) { ...@@ -350,24 +322,13 @@ void PlatformThread::SetCurrentThreadPriority(ThreadPriority priority) {
#if DCHECK_IS_ON() #if DCHECK_IS_ON()
const BOOL success = const BOOL success =
#endif #endif
::SetThreadPriority(thread_handle, desired_priority); ::SetThreadPriority(PlatformThread::CurrentHandle().platform_handle(),
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;
if (use_thread_mode_background && priority == ThreadPriority::BACKGROUND) { // Sanity check that GetCurrentThreadPriority() is consistent with
// In a background process, THREAD_MODE_BACKGROUND_BEGIN lowers the memory // SetCurrentThreadPriority().
// and I/O priorities but not the CPU priority (kernel bug?). Use
// THREAD_PRIORITY_LOWEST to also lower the CPU priority.
// https://crbug.com/901483
if (GetCurrentThreadPriority() != ThreadPriority::BACKGROUND) {
::SetThreadPriority(thread_handle, THREAD_PRIORITY_LOWEST);
// Make sure that using THREAD_PRIORITY_LOWEST didn't affect the memory
// priority set by THREAD_MODE_BACKGROUND_BEGIN. There is no practical
// way to verify the I/O priority.
internal::AssertMemoryPriority(thread_handle, MEMORY_PRIORITY_VERY_LOW);
}
}
DCHECK_EQ(GetCurrentThreadPriority(), priority); DCHECK_EQ(GetCurrentThreadPriority(), priority);
} }
......
...@@ -11,7 +11,6 @@ ...@@ -11,7 +11,6 @@
#include "base/feature_list.h" #include "base/feature_list.h"
namespace base { namespace base {
namespace features { namespace features {
// Use THREAD_MODE_BACKGROUND_BEGIN instead of THREAD_PRIORITY_LOWEST for // Use THREAD_MODE_BACKGROUND_BEGIN instead of THREAD_PRIORITY_LOWEST for
...@@ -22,15 +21,6 @@ namespace features { ...@@ -22,15 +21,6 @@ namespace features {
BASE_EXPORT extern const Feature kWindowsThreadModeBackground; BASE_EXPORT extern const Feature kWindowsThreadModeBackground;
} // namespace features } // namespace features
namespace internal {
// Assert that the memory priority of |thread| is |memory_priority|.
// No-op on Windows 7 because ::GetThreadInformation() is not available.
BASE_EXPORT void AssertMemoryPriority(HANDLE thread, int memory_priority);
} // namespace internal
} // namespace base } // namespace base
#endif // BASE_THREADING_PLATFORM_THREAD_WIN_H_ #endif // BASE_THREADING_PLATFORM_THREAD_WIN_H_
// 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.
#include "base/threading/platform_thread_win.h"
#include <Windows.h>
#include "base/process/process.h"
#include "base/win/windows_version.h"
#include "testing/gtest/include/gtest/gtest.h"
namespace base {
// It has been observed that calling
// :SetThreadPriority(THREAD_MODE_BACKGROUND_BEGIN) in a IDLE_PRIORITY_CLASS
// process has no effect on the return value of ::GetThreadPriority() or on the
// base priority reported in Process Explorer on Windows 8+. It does however set
// the memory and I/O priorities to very low. This test confirms that behavior
// which we suspect is a Windows kernel bug. If this test starts failing, the
// mitigation for https://crbug.com/901483 in
// PlatformThread::SetCurrentThreadPriority() should be revisited.
TEST(PlatformThreadWinTest, SetBackgroundThreadModeFailsInIdlePriorityProcess) {
PlatformThreadHandle::Handle thread_handle =
PlatformThread::CurrentHandle().platform_handle();
// ::GetThreadPriority() is NORMAL. Memory priority is NORMAL.
// Note: There is no practical way to verify the I/O priority.
EXPECT_EQ(THREAD_PRIORITY_NORMAL, ::GetThreadPriority(thread_handle));
internal::AssertMemoryPriority(thread_handle, MEMORY_PRIORITY_NORMAL);
// Set the process priority to IDLE.
// Note: Do not use Process::SetProcessBackgrounded() because it uses
// PROCESS_MODE_BACKGROUND_BEGIN instead of IDLE_PRIORITY_CLASS when
// the target process is the current process.
EXPECT_EQ(static_cast<DWORD>(NORMAL_PRIORITY_CLASS),
::GetPriorityClass(Process::Current().Handle()));
::SetPriorityClass(Process::Current().Handle(), IDLE_PRIORITY_CLASS);
// GetThreadPriority() stays NORMAL. Memory priority stays NORMAL.
EXPECT_EQ(THREAD_PRIORITY_NORMAL, ::GetThreadPriority(thread_handle));
internal::AssertMemoryPriority(thread_handle, MEMORY_PRIORITY_NORMAL);
// Begin thread mode background.
EXPECT_TRUE(::SetThreadPriority(thread_handle, THREAD_MODE_BACKGROUND_BEGIN));
// GetThreadPriority() becomes IDLE on Win 7 but stays normal on Win 8+.
// Memory priority becomes VERY_LOW.
//
// Note: this documents the aforementioned Windows 8+ kernel bug. Ideally this
// would *not* be the case.
if (win::GetVersion() == win::VERSION_WIN7)
EXPECT_EQ(THREAD_PRIORITY_IDLE, ::GetThreadPriority(thread_handle));
else
EXPECT_EQ(THREAD_PRIORITY_NORMAL, ::GetThreadPriority(thread_handle));
internal::AssertMemoryPriority(thread_handle, MEMORY_PRIORITY_VERY_LOW);
// Set the process priority to NORMAL.
::SetPriorityClass(Process::Current().Handle(), NORMAL_PRIORITY_CLASS);
// GetThreadPriority() stays NORMAL on Win 8+. Memory priority stays VERY_LOW.
if (win::GetVersion() == win::VERSION_WIN7)
EXPECT_EQ(THREAD_PRIORITY_IDLE, ::GetThreadPriority(thread_handle));
else
EXPECT_EQ(THREAD_PRIORITY_NORMAL, ::GetThreadPriority(thread_handle));
internal::AssertMemoryPriority(thread_handle, MEMORY_PRIORITY_VERY_LOW);
// End thread mode background.
//
// Note: at least "ending" the semi-enforced background mode works...
EXPECT_TRUE(::SetThreadPriority(thread_handle, THREAD_MODE_BACKGROUND_END));
// GetThreadPriority() stays NORMAL. Memory priority becomes NORMAL.
EXPECT_EQ(THREAD_PRIORITY_NORMAL, ::GetThreadPriority(thread_handle));
internal::AssertMemoryPriority(thread_handle, MEMORY_PRIORITY_NORMAL);
}
} // namespace base
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