Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1870033002:
Fix NiceValueToThreadPriority failing on an unknown nice value. (Closed)
|
Created:
4 years, 8 months ago by Yuta Kitamura Modified:
4 years, 8 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix NiceValueToThreadPriority failing on an unknown nice value.
Previously, NiceValueToThreadPriority assumed that the nice value of a
process is one of the predefined set of values, and reached NOTREACHED()
when the user set any other nice value.
This patch fixes this issue by changing the lookup of the priorities
more graceful. Now the function chooses the most reasonable priority
from the given nice value.
BUG=558314
R=gab@chromium.org
Committed: https://crrev.com/445e39d83dfc06a3f40b8547207cd9e12c3a4d3e
Cr-Commit-Position: refs/heads/master@{#387280}
Patch Set 1 #Patch Set 2 : Fix comments. #
Total comments: 6
Patch Set 3 : Apply gab's comments. #
Total comments: 7
Patch Set 4 : One more BASE_EXPORT to fix builds. #Patch Set 5 : Address Dana's comments. #Patch Set 6 : Don't run tests on MACOSX or IOS. #
Total comments: 2
Patch Set 7 : Add a comment about exclusion of OSX/iOS. #
Messages
Total messages: 34 (13 generated)
The CQ bit was checked by yutak@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1870033002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1870033002/1
The CQ bit was checked by yutak@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1870033002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1870033002/20001
gab: PTAL? I admit I was too lazy to define the tests on all the platforms. The tests should probably be expanded for other OSes, but I would like to hear from you about this patch anyway.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks, lgtm % request to take advantage of structure's order. https://codereview.chromium.org/1870033002/diff/20001/base/threading/platform... File base/threading/platform_thread_internal_posix.cc (right): https://codereview.chromium.org/1870033002/diff/20001/base/threading/platform... base/threading/platform_thread_internal_posix.cc:26: // an exact match, we return the priority whose nice value is closest to 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) https://codereview.chromium.org/1870033002/diff/20001/base/threading/platform... base/threading/platform_thread_internal_posix.cc:33: kThreadPriorityToNiceValueMap) { 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.
https://codereview.chromium.org/1870033002/diff/20001/base/threading/platform... File base/threading/platform_thread_unittest.cc (right): https://codereview.chromium.org/1870033002/diff/20001/base/threading/platform... base/threading/platform_thread_unittest.cc:278: EXPECT_EQ(ThreadPriority::BACKGROUND, NiceValueToThreadPriority(19)); To make these cross-platform (well on OS_POSIX) you could extract the underlying value from the map (it's okay to include the internal header in tests IMO), would also make it clearer what's being tested IMO (e.g. NORMAL+1, etc.)
The CQ bit was checked by yutak@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1870033002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1870033002/40001
yutak@chromium.org changed reviewers: + danakj@chromium.org
gab: PTAL at patchset 2:3. danakj: PTAL as an OWNER. Thanks! https://codereview.chromium.org/1870033002/diff/20001/base/threading/platform... File base/threading/platform_thread_internal_posix.cc (right): https://codereview.chromium.org/1870033002/diff/20001/base/threading/platform... base/threading/platform_thread_internal_posix.cc:26: // an exact match, we return the priority whose nice value is closest to On 2016/04/08 15:50:49, gab wrote: > 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) Yeah, that's much better (I rewrote the phrase several times but failed to come up with a good one...). Fixed. https://codereview.chromium.org/1870033002/diff/20001/base/threading/platform... base/threading/platform_thread_internal_posix.cc:33: kThreadPriorityToNiceValueMap) { On 2016/04/08 15:50:49, gab wrote: > 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. Adopted your first code. Also added a comment for the requirement regarding the order of priorities. https://codereview.chromium.org/1870033002/diff/20001/base/threading/platform... File base/threading/platform_thread_unittest.cc (right): https://codereview.chromium.org/1870033002/diff/20001/base/threading/platform... base/threading/platform_thread_unittest.cc:278: EXPECT_EQ(ThreadPriority::BACKGROUND, NiceValueToThreadPriority(19)); On 2016/04/08 15:52:51, gab wrote: > To make these cross-platform (well on OS_POSIX) you could extract the underlying > value from the map (it's okay to include the internal header in tests IMO), > would also make it clearer what's being tested IMO (e.g. NORMAL+1, etc.) Yeah, updated the tests so they work on OS_POSIX.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
Very nice, lgtm++, thanks! https://codereview.chromium.org/1870033002/diff/40001/base/threading/platform... File base/threading/platform_thread_internal_posix.cc (right): https://codereview.chromium.org/1870033002/diff/40001/base/threading/platform... base/threading/platform_thread_internal_posix.cc:8: #include "base/logging.h" nit: empty line between C++ includes and Chrome includes https://codereview.chromium.org/1870033002/diff/40001/base/threading/platform... File base/threading/platform_thread_unittest.cc (right): https://codereview.chromium.org/1870033002/diff/40001/base/threading/platform... base/threading/platform_thread_unittest.cc:295: static const int kRealtimeAudioNiceValue = static is good for compound const locals (e.g. arrays/structs) as it avoids a copy at runtime but it's useless for non-compound types (e.g. int), please remove above and below
https://codereview.chromium.org/1870033002/diff/40001/base/threading/platform... File base/threading/platform_thread_internal_posix.cc (right): https://codereview.chromium.org/1870033002/diff/40001/base/threading/platform... base/threading/platform_thread_internal_posix.cc:31: for (const ThreadPriorityToNiceValuePair& pair : this should use const auto& https://codereview.chromium.org/1870033002/diff/40001/base/threading/platform... base/threading/platform_thread_internal_posix.cc:32: kThreadPriorityToNiceValueMap) { if you base::Reversed this to iterate opposite, you don't need to store a best_priority, right? You just return the first one that is >= nice_value. If you exit the loop without finding one you return background?
The CQ bit was checked by yutak@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1870033002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1870033002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
The CQ bit was checked by yutak@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1870033002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1870033002/100001
danakj: PTAL again? https://codereview.chromium.org/1870033002/diff/40001/base/threading/platform... File base/threading/platform_thread_internal_posix.cc (right): https://codereview.chromium.org/1870033002/diff/40001/base/threading/platform... base/threading/platform_thread_internal_posix.cc:8: #include "base/logging.h" On 2016/04/12 13:20:46, gab -- OOO this Wed wrote: > nit: empty line between C++ includes and Chrome includes Done. https://codereview.chromium.org/1870033002/diff/40001/base/threading/platform... base/threading/platform_thread_internal_posix.cc:31: for (const ThreadPriorityToNiceValuePair& pair : On 2016/04/12 21:02:26, danakj wrote: > this should use const auto& Done. https://codereview.chromium.org/1870033002/diff/40001/base/threading/platform... base/threading/platform_thread_internal_posix.cc:32: kThreadPriorityToNiceValueMap) { On 2016/04/12 21:02:26, danakj wrote: > if you base::Reversed this to iterate opposite, you don't need to store a > best_priority, right? You just return the first one that is >= nice_value. If > you exit the loop without finding one you return background? Yeah, that's much simpler. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/1870033002/diff/100001/base/threading/platfor... File base/threading/platform_thread_unittest.cc (right): https://codereview.chromium.org/1870033002/diff/100001/base/threading/platfor... base/threading/platform_thread_unittest.cc:275: #if defined(OS_POSIX) && !defined(OS_MACOSX) && !defined(OS_IOS) Why not mac or ios? Please leave a comment explaining.
Thanks! Landing. https://codereview.chromium.org/1870033002/diff/100001/base/threading/platfor... File base/threading/platform_thread_unittest.cc (right): https://codereview.chromium.org/1870033002/diff/100001/base/threading/platfor... base/threading/platform_thread_unittest.cc:275: #if defined(OS_POSIX) && !defined(OS_MACOSX) && !defined(OS_IOS) On 2016/04/13 18:43:00, danakj wrote: > Why not mac or ios? Please leave a comment explaining. Done.
The CQ bit was checked by yutak@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org, danakj@chromium.org Link to the patchset: https://codereview.chromium.org/1870033002/#ps120001 (title: "Add a comment about exclusion of OSX/iOS.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1870033002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1870033002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Fix NiceValueToThreadPriority failing on an unknown nice value. Previously, NiceValueToThreadPriority assumed that the nice value of a process is one of the predefined set of values, and reached NOTREACHED() when the user set any other nice value. This patch fixes this issue by changing the lookup of the priorities more graceful. Now the function chooses the most reasonable priority from the given nice value. BUG=558314 R=gab@chromium.org ========== to ========== Fix NiceValueToThreadPriority failing on an unknown nice value. Previously, NiceValueToThreadPriority assumed that the nice value of a process is one of the predefined set of values, and reached NOTREACHED() when the user set any other nice value. This patch fixes this issue by changing the lookup of the priorities more graceful. Now the function chooses the most reasonable priority from the given nice value. BUG=558314 R=gab@chromium.org Committed: https://crrev.com/445e39d83dfc06a3f40b8547207cd9e12c3a4d3e Cr-Commit-Position: refs/heads/master@{#387280} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/445e39d83dfc06a3f40b8547207cd9e12c3a4d3e Cr-Commit-Position: refs/heads/master@{#387280} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
