Fix NiceValueToThreadPriority failing on an unknown nice value.

base/threading/platform_thread_internal_posix.cc

Index: base/threading/platform_thread_internal_posix.cc
diff --git a/base/threading/platform_thread_internal_posix.cc b/base/threading/platform_thread_internal_posix.cc
index 9af02044fceed2635b4fc5bf1ee5e1a06927a6fe..b0919e4fa8705e00f3744c14456e722d60beb706 100644
--- a/base/threading/platform_thread_internal_posix.cc
+++ b/base/threading/platform_thread_internal_posix.cc
@@ -4,6 +4,7 @@
 
 #include "base/threading/platform_thread_internal_posix.h"
 
+#include <limits>
 #include "base/logging.h"
 
 namespace base {
@@ -21,13 +22,26 @@ int ThreadPriorityToNiceValue(ThreadPriority priority) {
 }
 
 ThreadPriority NiceValueToThreadPriority(int nice_value) {
+  // Try to find a priority that best describes |nice_value|. If there isn't
+  // an exact match, we return the priority whose nice value is closest to

[gab, 2016/04/08 15:50:49]

How about:

"we return the priority whose nice value is closest ..." 

=> "this method returns the closest priority whose nice value is higher (lower
priority) than |nice_value|."

(i.e. in the initial read I'd missed that you were only considering "among the
priorities whose nice values are more than |nice_value|" and thought this was
wrong)

[Yuta Kitamura, 2016/04/12 10:18:59]

Yeah, that's much better (I rewrote the phrase several times
but failed to come up with a good one...). Fixed.

+  // |nice_value| among the priorities whose nice values are more than
+  // |nice_value|.
+
+  ThreadPriority best_priority;
+  int best_nice_value = std::numeric_limits<int>::max();
   for (const ThreadPriorityToNiceValuePair& pair :
        kThreadPriorityToNiceValueMap) {

[gab, 2016/04/08 15:50:49]

If we add a requirement that the kThreadPriorityToNiceValueMap lists its
mappings from highest to lowest nice value (lowest to highest priority) on its
definition @ [1] (which is already true but needs to be made an API requirement
to depend on) than this code can be simplified as:

  for (const ThreadPriorityToNiceValuePair& pair :
       kThreadPriorityToNiceValueMap) {
    if (pair.nice_value >= nice_value) {
      DCHECK_LT(pair.nice_value, best_nice_value);
      best_priority = pair.priority;
      best_nice_value = pair.nice_value;
    } else {
      break;
    }
  }

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/pla...

Or to avoid the break:

  for (size_t i = 0; 
       kThreadPriorityToNiceValueMap[i].nice_value >= nice_value && 
       i < arraysize(kThreadPriorityToNiceValueMap); 
       ++i) {
    DCHECK_LT(kThreadPriorityToNiceValueMap[i].nice_value, best_nice_value);
    best_priority = kThreadPriorityToNiceValueMap[i].priority;
    best_nice_value = kThreadPriorityToNiceValueMap[i].nice_value;
  }

Don't feel strongly on which one of these you prefer, but I do think we should
take advantage of the order and document it as a requirement.

[Yuta Kitamura, 2016/04/12 10:18:59]

Adopted your first code. Also added a comment for the requirement
regarding the order of priorities.

-    if (pair.nice_value == nice_value)
-      return pair.priority;
+    if (pair.nice_value >= nice_value && pair.nice_value < best_nice_value) {
+      best_priority = pair.priority;
+      best_nice_value = pair.nice_value;
+    }
   }
-  NOTREACHED() << "Unknown nice value";
-  return ThreadPriority::NORMAL;
+  if (best_nice_value != std::numeric_limits<int>::max())
+    return best_priority;
+
+  // Reaching here means |nice_value| is more than any of the defined
+  // priorites. The lowest priority is suitable in this case.
+  return ThreadPriority::BACKGROUND;
 }
 
 }  // namespace internal