base: Implement GetCurrentThreadPriority.

base: Implement GetCurrentThreadPriority.

The Chrome thread priority is saved in the thread dictionary during
SetCurrentThreadPriority(). This seemed a better approach than using
thread_policy_get(), since there are 4 Chrome priorities which won't cleanly map
to thread flavors.

BUG=554651
Committed: https://crrev.com/42655dd168d884f60c0194e8a74606c68a3e6378
Cr-Commit-Position: refs/heads/master@{#381102}

[erikchen, 2016-01-23 01:35:52]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-23 01:37:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1620673003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1620673003/1

[commit-bot: I haz the power, 2016-01-23 01:47:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-23 01:47:42]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[erikchen, 2016-01-23 02:11:39]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-23 02:11:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1620673003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1620673003/20001

[commit-bot: I haz the power, 2016-01-23 03:18:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-23 03:18:03]

Dry run: This issue passed the CQ dry run.

[erikchen, 2016-01-25 20:05:50]

erikchen@chromium.org changed reviewers:
+ mark@chromium.org

[erikchen, 2016-01-25 20:05:51]

mark: Please review.

[Mark Mentovai, 2016-01-25 20:38:33]

What’s the point of doing this if all we’re actually doing is stashing a
meaningless number into a slot where nobody else will see it? Just to make a
test pass? Returning these other priorities from the getter kind of makes it
seem like it’s expected to actually work somehow.

Since all we really support is normal and realtime-audio, I think that those are
the only values we should return from the getter.

[erikchen, 2016-01-25 20:48:38]

On 2016/01/25 20:38:33, Mark Mentovai wrote:
> What’s the point of doing this if all we’re actually doing is stashing a
> meaningless number into a slot where nobody else will see it? Just to make a
> test pass? Returning these other priorities from the getter kind of makes it
> seem like it’s expected to actually work somehow.
> 
> Since all we really support is normal and realtime-audio, I think that those
are
> the only values we should return from the getter.

Right now, there is a cross-platform API that doesn't work on Mac
(base::PlatformThread::{Get,Set}CurrentThreadPriority). Consumers of base/
shouldn't need to wrap calls to that API in preprocessor conditionals.

e.g.
https://code.google.com/p/chromium/issues/detail?id=554651
https://code.google.com/p/chromium/codesearch#chromium/src/components/startup...

I agree with Gab's sentiment from the crbug that the fact that we only support
two thread priorities on Mac should be an implementation detail of base/. I was
going to write a follow up CL that changes the logic of
SetCurrentThreadPriority() to allow all 4 thread-priorities, even though three
of them will map to NORMAL. Would you prefer if I put everything into a single
CL?

[Mark Mentovai, 2016-01-25 21:24:24]

If the Set allows all 4, then this makes a little more sense. Without that, I
can’t see justification for the Get to return anything other than NORMAL and
REALTIME_AUDIO.

I guess that’s a vote for merging the changes.

[erikchen, 2016-01-26 18:45:29]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-26 18:47:15]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1620673003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1620673003/60001

[commit-bot: I haz the power, 2016-01-26 19:50:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-26 19:50:36]

Dry run: This issue passed the CQ dry run.

[erikchen, 2016-02-01 23:26:14]

On 2016/01/26 19:50:36, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

mark: PTAL

[Mark Mentovai, 2016-02-02 16:38:27]

LGTM

https://codereview.chromium.org/1620673003/diff/60001/base/threading/platform...
File base/threading/platform_thread_mac.mm (right):

https://codereview.chromium.org/1620673003/diff/60001/base/threading/platform...
base/threading/platform_thread_mac.mm:27: NSString* const kThreadPriorityKey =
@"kThreadPriorityKey";
No leading k in the string on the right side. And actually, let’s namespace it
to ourselves: CrThreadPriorityKey.

https://codereview.chromium.org/1620673003/diff/60001/base/threading/platform...
base/threading/platform_thread_mac.mm:183:
setObject:@(static_cast<int>(priority))
OK, but I’d be even happier about this if the ”enum class ThreadPriority” were
“: int”.

[erikchen, 2016-03-14 21:09:49]

https://codereview.chromium.org/1620673003/diff/60001/base/threading/platform...
File base/threading/platform_thread_mac.mm (right):

https://codereview.chromium.org/1620673003/diff/60001/base/threading/platform...
base/threading/platform_thread_mac.mm:27: NSString* const kThreadPriorityKey =
@"kThreadPriorityKey";
On 2016/02/02 16:38:27, Mark Mentovai wrote:
> No leading k in the string on the right side. And actually, let’s namespace it
> to ourselves: CrThreadPriorityKey.

Done.

https://codereview.chromium.org/1620673003/diff/60001/base/threading/platform...
base/threading/platform_thread_mac.mm:183:
setObject:@(static_cast<int>(priority))
On 2016/02/02 16:38:27, Mark Mentovai wrote:
> OK, but I’d be even happier about this if the ”enum class
ThreadPriority”&nbsp;were
> “: int”.

Done.

[erikchen, 2016-03-14 21:12:19]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2016-03-14 21:12:20]

The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org
Link to the patchset: https://codereview.chromium.org/1620673003/#ps100001
(title: "Rebase error.")

[erikchen, 2016-03-14 21:12:20]

The CQ bit was unchecked by erikchen@chromium.org

[erikchen, 2016-03-14 21:12:43]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2016-03-14 21:12:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1620673003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1620673003/100001

[commit-bot: I haz the power, 2016-03-14 22:54:25]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-03-14 22:55:20]

Description was changed from

==========
base: Implement GetCurrentThreadPriority.

The Chrome thread priority is saved in the thread dictionary during
SetCurrentThreadPriority(). This seemed a better approach than using
thread_policy_get(), since there are 4 Chrome priorities which won't cleanly map
to thread flavors.

BUG=554651
==========

to

==========
base: Implement GetCurrentThreadPriority.

The Chrome thread priority is saved in the thread dictionary during
SetCurrentThreadPriority(). This seemed a better approach than using
thread_policy_get(), since there are 4 Chrome priorities which won't cleanly map
to thread flavors.

BUG=554651

Committed: https://crrev.com/42655dd168d884f60c0194e8a74606c68a3e6378
Cr-Commit-Position: refs/heads/master@{#381102}
==========

[commit-bot: I haz the power, 2016-03-14 22:55:21]

Patchset 6 (id:??) landed as
https://crrev.com/42655dd168d884f60c0194e8a74606c68a3e6378
Cr-Commit-Position: refs/heads/master@{#381102}

[gab, 2016-03-15 02:37:06]

gab@chromium.org changed reviewers:
+ gab@chromium.org

[gab, 2016-03-15 02:37:07]

Woot! Thanks for doing this :-), minor nit/tip as I glanced over the code out of
curiosity.

Cheers,
Gab

https://codereview.chromium.org/1620673003/diff/100001/base/threading/platfor...
File base/threading/platform_thread_mac.mm (right):

https://codereview.chromium.org/1620673003/diff/100001/base/threading/platfor...
base/threading/platform_thread_mac.mm:205: return ThreadPriority::NORMAL;
nit: Prefer putting these two lines outside the switch statement and removing
the "default" statement. That way if a new ThreadPriority is added, clang will
throw an error when compiling the switch instead of silently providing an
undesired implementation.