Add full SetThreadPriority support to Windows.

base/threading/platform_thread_win.cc

Index: base/threading/platform_thread_win.cc
diff --git a/base/threading/platform_thread_win.cc b/base/threading/platform_thread_win.cc
index 3df371943f5e6fdf1a91175802b7fb36f78d9aba..3759b705042305280a6679e9941f6ef0a48fca4e 100644
--- a/base/threading/platform_thread_win.cc
+++ b/base/threading/platform_thread_win.cc
@@ -126,18 +126,17 @@ bool CreateThreadInternal(size_t stack_size,
 
 // static
 PlatformThreadId PlatformThread::CurrentId() {
-  return GetCurrentThreadId();
+  return ::GetCurrentThreadId();
 }
 
 // static
 PlatformThreadRef PlatformThread::CurrentRef() {
-  return PlatformThreadRef(GetCurrentThreadId());
+  return PlatformThreadRef(::GetCurrentThreadId());
 }
 
 // static
 PlatformThreadHandle PlatformThread::CurrentHandle() {
-  NOTIMPLEMENTED(); // See OpenThread()
-  return PlatformThreadHandle();
+  return PlatformThreadHandle(::GetCurrentThread());

[rvargas (doing something else), 2015/03/19 22:19:21]

Sounds like it will leak.

[gab, 2015/03/30 20:14:45]

::GetCurrentThread() returns a pseudo-handle which isn't leakable AFAIK.

[rvargas (doing something else), 2015/03/30 22:31:23]

Yes, but now you have a valid PlatformThreadHandle that cannot be used anywhere
but the current thread. Sounds like a broken implementation to me.

[gab, 2015/03/31 14:02:33]

Right,  actually adjusted the API comment on PlatformThread::CurrentHandle() to
reflect that.

We can't Duplicate the pseudo-handle to get a real one either as then it would
leak -- this entire handle API seems fragile, but this is outside the scope of
this CL IMO.

[rvargas (doing something else), 2015/03/31 19:27:09]

I'm not really sure what's the right hing to do now. There's exactly 3 users of
CurrentHandle() today: two of them are Android calls to SetThreadPriority
(tightly coupled with this CL) and the last one is CompositorOutputSurface
setting main_thread_handle_, with a TODO to implement on windows... which leads
me to believe that adding a comment about the handle not being usable from
another thread is not going to help the only user of this call.
Yeah, that's why I CC'd you on the bug I filed about removing this whole thing.

So while I agree that changing the usage model is clearly out of scope, I'm not
convinced that we are vested enough in preserving the current model (almost no
use!). From that point of view, having NOTIMPLEMENTED here is a very strong
signal that something is broken, while returning the pseudo-handle creates the
impression that things are OK.

I would prefer not implementing this method and instead working around it in
your code. At least it is less work when removing this class, and doesn't
encourage external use.

[gab, 2015/03/31 20:18:20]

I'm not convinced that it not applying to existing use cases warrants hardcoding
hacks in places it could now be useful (the next step for me is to experiment
with backgrounding threads which may very well need a cross-platform way of
expressing "current thread").

I agree however that restricting the API for all platforms hurts existing use
cases, so I made the API comment Windows specific, how about that?

TBH, I wouldn't worry too much about its usage spreading wildly. If anything I
think this comment makes it clearer (i.e., I disagree that keeping a
NONIMPLEMENTED() on Windows hidden behind an API that doesn't mention it will
help keep callers away).

[rvargas (doing something else), 2015/03/31 22:43:44]

I agree that the current situation is not good because there is no indication
that the API is not implemented everywhere (other than the obvious crash as soon
as someone tries to use it on multiplatform code!). So a comment is a welcomed
addition. (to be fair, having NOTIMPLEMENTED without a note on the header is
fairly common).

But... the current status is fairly limiting to anyone that tries to use this,
which is a good thing because the API as it is seems broken. I'd rather remove
the API completely than move to a more imbalanced state: I don't think we should
have base exposing APIs that behave quite different per platform (x can use from
any thread, y cannot).

So if you believe that you need this functionality for your next CL, and that
what you want is a pseudo-handle that can only be used from the current thread,
then I'd suggest renaming the method to be more obvious about it
("PseudoHandleForCurrenThread()", I'm bad with names). That will fit your new
code and two out of the three current uses, and hopefully provide a strong hint
about not using it to identify threads.

I don't know what to do about the third one though... it probably needs a real
handle, and the API needs a way to close that handle (which should be a
move-only RAAI type so PlatformThreadHandle is not a good fit). I would be fine
using the same code for the part that is implemented (remember that said code is
not finished as it presumably doesn't work on Windows), and adding yet another
TODO to do The Right Thing later.

[gab, 2015/04/01 01:14:39]

How about we make a single call that returns an object representing a
pseudo-handle which exposes a GetThreadSafeCopy() method which itself returns
real handle inside an RAII object?

i.e., something like:

class PlatformThreadHandle; (pure-virtual - move-only RAII type)

class PlatformThreadPseudoHandle : public PlatformThreadHandle,
                                   public NonThreadSafe {
 public:
  Handle handle() override { CalledOnValidThread(); return pseudo_handle_; }

  PlatformThreadHandle GetThreadSafeCopy() { return
PlatformThreadThreadSafeHandle(*this); }
}

class PlatformThreadThreadSafeHandle : public PlatformThreadHandle {
 public:
  PlatformThreadThreadSafeHandle(const PlatformThreadHandle& other) {
   (duplicate |other|...)
  }
  
  ~PlatformThreadThreadSafeHandle() { (release handle...) }

  Handle handle() override { return handle_; }
}



This way most users don't pay the cost of obtaining the real handle and those
that truly need one, can happily get one?

Feels outside the scope of this CL, happy to attack this as a precursor or
follow-up, whichever your prefer.

[rvargas (doing something else), 2015/04/01 02:04:55]

How does this play in the context of crbug.com/468932 ?

I mean, I like the idea of something that is implicitly pseudoish, but I'm not
sure about PlatformThreadHandle (it's mostly a matter of naming, I guess). I
sort of think that base::Thread should be the real object (to parallel
base::Process). 

Note that the way Process deals with this issue is by an internal bool that
keeps track of the object being a real handle or a pseudo handle; that way
callers mostly don't care about what they have. The issue in this case is that
CurrentThread is more subtle in that just passing it to another thread makes it
invalid... hence the allure of a subclass. (but NonThreadSafe objects in general
can be created on a different thread than where they are used all the time).

Anyways, yes, that doesn't belong here. Ideally, we should start moving in a
better direction sort of in parallel. We are adding new APIs and we might as
well add them were we think they should live long term.

In that context, it can be post or pre- this CL... with the caveat that just
looking at this CL, there are no new calls of PlatformThread::CurrentHandle()
(right?) so it's not that we _have_ to implement this on Windows as part of this
CL.

[gab, 2015/04/01 14:46:50]

Non-thread-safe objects yes, but not implementers of NonThreadSafe AFAIK (unless
they call DetachFromThread() which stops the checking and kind of defeats the
purpose).
It is used in this CL (in the new tests), I can have Windows if'defs there for
now instead of fixing the Windows impl in this CL, up to you.

[rvargas (doing something else), 2015/04/01 16:31:30]

How about this: if PlatformThread::CurrentHandle is fixed/replaced in less than
say, two or three weeks then go ahead and just return ::GetCurrentThread. If the
fix may take longer than that then let's go with ifdefs on the test.

[gab, 2015/04/01 18:12:19]

Okay, I will work on this next, expect a CL soon.

 }
 
 // static
@@ -234,17 +233,85 @@ void PlatformThread::Join(PlatformThreadHandle thread_handle) {
 // static
 void PlatformThread::SetThreadPriority(PlatformThreadHandle handle,
                                        ThreadPriority priority) {
+  DCHECK(!handle.is_null());
+
+  if (base::win::GetVersion() >= base::win::VERSION_VISTA &&
+      handle.is_equal(CurrentHandle()) &&
+      priority != kThreadPriority_Background) {
+    // Make sure to end any active background mode.
+    bool unset =

[rvargas (doing something else), 2015/03/19 22:19:21]

BOOL?

[rvargas (doing something else), 2015/03/19 22:19:21]

BOOL?

[gab, 2015/03/30 20:14:45]

Done.

[gab, 2015/03/30 20:14:45]

Done.

+        !!::SetThreadPriority(handle.handle_, THREAD_MODE_BACKGROUND_END);

[rvargas (doing something else), 2015/03/19 22:19:21]

... I really hate !! to convert BOOL to bool

[gab, 2015/03/30 20:14:45]

Done.

+    DPCHECK(unset ||
+            ERROR_THREAD_MODE_NOT_BACKGROUND ==
+                static_cast<int>(::GetLastError()));
+  }
+
+  bool success = false;
   switch (priority) {
     case kThreadPriority_Normal:
-      ::SetThreadPriority(handle.handle_, THREAD_PRIORITY_NORMAL);
+      success = !!::SetThreadPriority(handle.handle_, THREAD_PRIORITY_NORMAL);
       break;
     case kThreadPriority_RealtimeAudio:
-      ::SetThreadPriority(handle.handle_, THREAD_PRIORITY_TIME_CRITICAL);
+      success =
+          !!::SetThreadPriority(handle.handle_, THREAD_PRIORITY_TIME_CRITICAL);
+      break;
+    case kThreadPriority_Display:
+      success =
+          !!::SetThreadPriority(handle.handle_, THREAD_PRIORITY_ABOVE_NORMAL);
+      break;
+    case kThreadPriority_Background:
+      if (base::win::GetVersion() >= base::win::VERSION_VISTA &&
+          handle.is_equal(CurrentHandle())) {
+        // Full background mode (low IO/memory on top of low CPU priority) can
+        // only be set from the current thread.
+        success =

[rvargas (doing something else), 2015/03/19 22:19:21]

This is problematic as this is a non-reentrant property.

Calling SetPriority(background) twice is not forbidden by our API (and I don't
really think it should be).

[gab, 2015/03/30 20:14:45]

Good point, fixed.

+            !!::SetThreadPriority(handle.handle_, THREAD_MODE_BACKGROUND_BEGIN);
+      } else {
+        success = !!::SetThreadPriority(handle.handle_, THREAD_PRIORITY_LOWEST);

[gab, 2015/03/19 18:06:07]

One question, do you think we should even allow background mode to be set from
other threads or should we instead DCHECK here to make sure callers know to use
the path resulting in the best behaviour (for now this is only mentioned in the
API, but not enforced).

[rvargas (doing something else), 2015/03/19 22:19:21]

We should either allow any priority to be set from anywhere, or document that
priorities should only be set from the same thread.

[gab, 2015/03/30 20:14:45]

Ok, for now going with the simple change to get the architecture in, will
consider a more advanced background in a follow-up.

+      }
       break;
     default:
       NOTREACHED() << "Unknown priority.";
       break;
   }
+  DPCHECK(success);

[rvargas (doing something else), 2015/03/19 22:19:21]

Should not check on a OS function return (and elsewhere)

[gab, 2015/03/30 20:14:45]

To me the goal of a debug check is to find out if this doesn't behave as
expected in some configurations (rather than silently fail) on some bots or some
dev machines -- or even on new OS versions as they release.

[rvargas (doing something else), 2015/03/30 22:31:22]

A DCHECK is by definition some invariant of the code. An invariant is something
that is somehow under control of the developer that is calling this function.

The error code of an operation performed by the OS (or any form of status saying
if the operation failed or not), is way outside the things that are under
control of the developer.

I feel very strongly against this practice... it just leads to debug builds that
are very hard to use as random DCHEKS hit all the time.

[gab, 2015/03/31 14:02:33]

Okay changed to use DPLOG_IF(ERROR, !success)

[gab, 2015/03/31 18:35:25]

But FWIW, I'm more in favor of explicitly checking OS behaviours that we rely
on. There is precedent for such checks, e.g.:
https://code.google.com/p/chromium/codesearch#chromium/src/base/synchronizati...

[rvargas (doing something else), 2015/03/31 19:27:09]

Well played, sir. I should have removed that one.

+}
+
+// static
+ThreadPriority PlatformThread::GetThreadPriority(PlatformThreadHandle handle) {
+  DCHECK(!handle.is_null());
+
+  if (base::win::GetVersion() >= base::win::VERSION_VISTA &&
+      handle.is_equal(CurrentHandle())) {
+    // The only way to test for background mode is to explicitly try to exit it

[rvargas (doing something else), 2015/03/19 22:19:21]

Why not always set the priority to lowest (any os) and ignore this state in this
function?

[gab, 2015/03/30 20:14:46]

Well because we desire to use true background mode to get background IO/memory
priority as well, but I decided to strip that out in this CL which already has
enough structural changes, I'll consider bringing it back on its own or under a
different form in a follow-up CL (PS: I don't understand why Windows' API is
dumb and only works from current thread...)

[rvargas (doing something else), 2015/03/30 22:31:23]

That's not what I meant. What I meant was that we could:
SetThreadPriority(kThreadPriority_Background) {
  SetThreadPriority(THREAD_PRIORITY_LOWEST);
  If (we can) SetThreadPriority(THREAD_MODE_BACKGROUND_BEGIN);
}

GetThreadPriority() {
  if (THREAD_PRIORITY_LOWEST) return kThreadPriority_Background;
}

re PS: I suspect someone decided that injecting a thread to set other threads to
background io was not a good thing. But asymmetric APIs are a pain. 

[gab, 2015/03/31 14:02:33]

Ah I see, still think it will be easier to do background mode in a separate CL.

[rvargas (doing something else), 2015/03/31 19:27:09]

Acknowledged.

+    // (success meaning it was enabled)...
+    if (::SetThreadPriority(handle.handle_, THREAD_MODE_BACKGROUND_END)) {
+      ::SetThreadPriority(handle.handle_, THREAD_MODE_BACKGROUND_BEGIN);
+      return kThreadPriority_Background;
+    } else {
+      // The only expected failure condition to the above test is not being in
+      // background mode in the first place.
+      DCHECK_EQ(ERROR_THREAD_MODE_NOT_BACKGROUND,
+                static_cast<int>(::GetLastError()));
+    }
+  }
+
+  int priority = ::GetThreadPriority(handle.handle_);
+  switch (priority) {
+    case THREAD_PRIORITY_NORMAL:
+      return kThreadPriority_Normal;
+    case THREAD_PRIORITY_TIME_CRITICAL:
+      return kThreadPriority_RealtimeAudio;
+    case THREAD_PRIORITY_ABOVE_NORMAL:
+      return kThreadPriority_Display;
+    case THREAD_PRIORITY_LOWEST:
+      return kThreadPriority_Background;
+    case THREAD_PRIORITY_ERROR_RETURN:
+      DPCHECK(false) << "GetThreadPriority error";  // Falls through.
+    default:
+      NOTREACHED() << "Unexpected priority: " << priority;
+      return kThreadPriority_Normal;
+  }
 }
 
 }  // namespace base