Deduplicate PlatformThread's POSIX implementations of SetThreadPriority.

base/threading/platform_thread_posix.cc

 // Copyright (c) 2012 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.h"
 
 #include <errno.h>
+#include <pthread.h>
 #include <sched.h>
+#include <sys/resource.h>
+#include <sys/time.h>
 
 #include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/safe_strerror_posix.h"
 #include "base/synchronization/waitable_event.h"
+#include "base/threading/platform_thread_internal_posix.h"
 #include "base/threading/thread_id_name_manager.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/tracked_objects.h"
 
-#if defined(OS_MACOSX)
-#include <sys/resource.h>
-#include <algorithm>
-#endif
-
 #if defined(OS_LINUX)
-#include <sys/prctl.h>
-#include <sys/resource.h>
 #include <sys/syscall.h>
-#include <sys/time.h>
-#include <unistd.h>
+#elif defined(OS_ANDROID)
+#include <sys/types.h>
 #endif
 
 namespace base {
 
 void InitThreading();
 void InitOnThread();
 void TerminateOnThread();
 size_t GetDefaultThreadStackSize(const pthread_attr_t& attributes);
 
 namespace {
@@ skipping 188 matching lines @@
 
 // static
 void PlatformThread::Join(PlatformThreadHandle thread_handle) {
   // Joining another thread may block the current thread for a long time, since
   // the thread referred to by |thread_handle| may still be running long-lived /
   // blocking tasks.
   base::ThreadRestrictions::AssertIOAllowed();
   CHECK_EQ(0, pthread_join(thread_handle.handle_, NULL));
 }
 
+// Mac has its own SetThreadPriority() implementation.
+#if !defined(OS_MACOSX)
+// static
+void PlatformThread::SetThreadPriority(PlatformThreadHandle handle,
+                                       ThreadPriority priority) {
+#if !defined(OS_NACL)
+  if (internal::HandleSetThreadPriorityForPlatform(handle, priority))
+    return;
+
+  // setpriority(2) should change the whole thread group's (i.e. process)
+  // priority. However, under the current Linux/NPTL implementation of POSIX

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

tiny nit: Remove the Linux part?

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

This was copy-pasted verbatim from http://linux.die.net/man/2/getpriority.

I'm no POSIX expert and mostly only trying to move some code around without
affecting existing logic and doing a best attempt at documentation in the
process (and would thus rather mention existing documentation verbatim unless
you feel strongly otherwise).

[rvargas (doing something else), 2015/03/18 22:54:23]

Yes, and that's what the comment said on the _linux.cc file... my issue is that
this is now a _posix.cc file so it looks weird to do things justified by Linux
and not posix (NPTL is fine) (I don't have an issue with the link pointing to
Linux; better if it was not, but that's hard!).

So I'm just saying that if we are generalizing this code for all posix OSes
(minus Mac variants), we shouldn't have to explain the behavior of one of them
(either all or none, no?). And yes, this is not a change in behavior, but that
part of the comment would feel foreign on the _bsd.cc or _mac.xx files :)

TBH I don't really feel that strongly about it. If you still want to keep it as
is, then add the link and treat it as quoted from that place (indent? quotes?).

[gab, 2015/03/19 15:32:50]

Fair enough, tweaked comment.

+  // threads, the nice value is a per-thread attribute. See the bugs section in
+  // http://man7.org/linux/man-pages/man2/getpriority.2.html. We prefer using 0
+  // rather than the current thread id since they are equivalent but it makes

[rvargas (doing something else), 2015/03/17 22:15:22]

is using 0 also equivalent on bsd? just checking

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

I asssume so, the only reason the current implementations diverge today IMO is
that the Linux impl was copied to FreeBSD impl in http://crrev.com/256124 and
the Linux/android impl were later changed independently in
http://crrev.com/286964 without the changes being reflected in the freebsd impl.

FreeBSD documentation backing this up:
https://www.freebsd.org/cgi/man.cgi?query=getpriority&sektion=2

[rvargas (doing something else), 2015/03/18 22:54:23]

Acknowledged.

+  // sandboxing easier (https://crbug.com/399473).
+  DCHECK_NE(handle.id_, kInvalidThreadId);
+  const int nice_setting = internal::ThreadPriorityToNiceValue(priority);

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

nit: either remove const (preferred) or name the variables as constant.

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

If by "name the variables as constant" you mean kNiceSetting, this nomenclature
is reserved for compile-time constants AFAIK.

And I like keeping const here as it's a hint to the reader that this value is
computed once and then used as-is elsewhere.

[rvargas (doing something else), 2015/03/18 22:54:23]

ok, up to you.

That's kind of a pet peeve of mine because that general argument can also be
used for arguments passed by value (if they are not modified), and that is
clearly against the style.

... and we end up in this case with four lines of real code (including one log
and three functions calls), with two variables (just for the log) that are not
modified by the last two lines and so they are const :(

+  const PlatformThreadId current_id = PlatformThread::CurrentId();
+  if (setpriority(PRIO_PROCESS, handle.id_ == current_id ? 0 : handle.id_,
+                  nice_setting)) {
+    DVPLOG(1) << "Failed to set nice value of thread (" << handle.id_ << ") to "
+              << nice_setting;
+  }
+#endif  // !defined(OS_NACL)
+}
+#endif  // !defined(OS_MACOSX)
+
 }  // namespace base