Deduplicate PlatformThread's POSIX implementations of SetThreadPriority.

Deduplicate PlatformThread's POSIX implementations of SetThreadPriority.

This deduplication becomes a particularly good win when implementing
GetThreadPriority() in https://codereview.chromium.org/1006933003/

BUG=456903, 468793
Committed: https://crrev.com/bb0d0b93645861e9e4dc66d31b674fdf2965ec4e
Cr-Commit-Position: refs/heads/master@{#321403}

[gab, 2015-03-17 20:12:51]

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

[gab, 2015-03-17 20:12:52]

Hey Ricardo,

I'm trying to add full SetThreadPriority() support on Windows
(https://codereview.chromium.org/1006933003/) + cross-platform tests of this
code, but first I'd like to refactor the heavily duplicated code for the POSIX
impls in order to be able to implement the new GetThreadPriority() call in one
rather than 3 copies for POSIX.

While I'm fighting with making this link, I'd like your take on whether you
think this approach is correct/viable (or whether you prefer code duplication
over all 3 linux/freebsd/android impls).

Thanks,
Gab

[rvargas (doing something else), 2015-03-17 22:13:03]

On 2015/03/17 20:12:52, gab wrote:
> Hey Ricardo,
> 
> I'm trying to add full SetThreadPriority() support on Windows
> (https://codereview.chromium.org/1006933003/) + cross-platform tests of this
> code, but first I'd like to refactor the heavily duplicated code for the POSIX
> impls in order to be able to implement the new GetThreadPriority() call in one
> rather than 3 copies for POSIX.
> 
> While I'm fighting with making this link, I'd like your take on whether you
> think this approach is correct/viable (or whether you prefer code duplication
> over all 3 linux/freebsd/android impls).
> 
> Thanks,
> Gab

I think the direction is fine.

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

https://codereview.chromium.org/1015503002/diff/100001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/1015503002/diff/100001/base/BUILD.gn#newcode488
base/BUILD.gn:488: "threading/platform_thread_internal_posix.cc",
twice

https://codereview.chromium.org/1015503002/diff/100001/base/threading/platfor...
File base/threading/platform_thread_android.cc (right):

https://codereview.chromium.org/1015503002/diff/100001/base/threading/platfor...
base/threading/platform_thread_android.cc:41: {kThreadPriority_RealtimeAudio,
-16},
nit: spaces after { and before }

https://codereview.chromium.org/1015503002/diff/100001/base/threading/platfor...
File base/threading/platform_thread_freebsd.cc (right):

https://codereview.chromium.org/1015503002/diff/100001/base/threading/platfor...
base/threading/platform_thread_freebsd.cc:27: const struct sched_param
kRealTimePrio = {8};
nit: { 8 }

https://codereview.chromium.org/1015503002/diff/100001/base/threading/platfor...
base/threading/platform_thread_freebsd.cc:40: // Handle real time priority the
Linux way if possible.
nit: This comment sounds weird. Why is pthread_setschedparam the linux way?

https://codereview.chromium.org/1015503002/diff/100001/base/threading/platfor...
File base/threading/platform_thread_internal_posix.cc (right):

https://codereview.chromium.org/1015503002/diff/100001/base/threading/platfor...
base/threading/platform_thread_internal_posix.cc:14: for (const
ThreadPriorityToNiceValuePair& tnp :
nit: don't abbreviate tnp

https://codereview.chromium.org/1015503002/diff/100001/base/threading/platfor...
File base/threading/platform_thread_posix.cc (right):

https://codereview.chromium.org/1015503002/diff/100001/base/threading/platfor...
base/threading/platform_thread_posix.cc:245: // priority. However, under the
current Linux/NPTL implementation of POSIX
tiny nit: Remove the Linux part?

https://codereview.chromium.org/1015503002/diff/100001/base/threading/platfor...
base/threading/platform_thread_posix.cc:248: // rather than the current thread
id since they are equivalent but it makes
is using 0 also equivalent on bsd? just checking

https://codereview.chromium.org/1015503002/diff/100001/base/threading/platfor...
base/threading/platform_thread_posix.cc:251: const int nice_setting =
internal::ThreadPriorityToNiceValue(priority);
nit: either remove const (preferred) or name the variables as constant.

[gab, 2015-03-18 18:55:17]

Patchset #7 (id:120001) has been deleted

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

Thanks PTAL.

https://codereview.chromium.org/1015503002/diff/100001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/1015503002/diff/100001/base/BUILD.gn#newcode488
base/BUILD.gn:488: "threading/platform_thread_internal_posix.cc",
On 2015/03/17 22:15:21, rvargas wrote:
> twice

Oops!

https://codereview.chromium.org/1015503002/diff/100001/base/threading/platfor...
File base/threading/platform_thread_android.cc (right):

https://codereview.chromium.org/1015503002/diff/100001/base/threading/platfor...
base/threading/platform_thread_android.cc:41: {kThreadPriority_RealtimeAudio,
-16},
On 2015/03/17 22:15:21, rvargas wrote:
> nit: spaces after { and before }

I don't feel strongly either way, but this is the output of "git cl format" (it
even changes it back if I explicitly add spaces).

https://codereview.chromium.org/1015503002/diff/100001/base/threading/platfor...
File base/threading/platform_thread_freebsd.cc (right):

https://codereview.chromium.org/1015503002/diff/100001/base/threading/platfor...
base/threading/platform_thread_freebsd.cc:27: const struct sched_param
kRealTimePrio = {8};
On 2015/03/17 22:15:21, rvargas wrote:
> nit: { 8 }

As mentioned in another comment, this is the output of "git cl format" (up to
you..)

https://codereview.chromium.org/1015503002/diff/100001/base/threading/platfor...
base/threading/platform_thread_freebsd.cc:40: // Handle real time priority the
Linux way if possible.
On 2015/03/17 22:15:21, rvargas wrote:
> nit: This comment sounds weird. Why is pthread_setschedparam the linux way?

Removed comment, was trying to justify it somehow, but I don't actually know why
this special handling is required beyond that the previous code did it.

https://codereview.chromium.org/1015503002/diff/100001/base/threading/platfor...
File base/threading/platform_thread_internal_posix.cc (right):

https://codereview.chromium.org/1015503002/diff/100001/base/threading/platfor...
base/threading/platform_thread_internal_posix.cc:14: for (const
ThreadPriorityToNiceValuePair& tnp :
On 2015/03/17 22:15:21, rvargas wrote:
> nit: don't abbreviate tnp 

how about "pair"? Or do you prefer something verbose list
"priority_to_nice_value_pair"?

https://codereview.chromium.org/1015503002/diff/100001/base/threading/platfor...
File base/threading/platform_thread_posix.cc (right):

https://codereview.chromium.org/1015503002/diff/100001/base/threading/platfor...
base/threading/platform_thread_posix.cc:245: // priority. However, under the
current Linux/NPTL implementation of POSIX
On 2015/03/17 22:15:21, rvargas wrote:
> tiny nit: Remove the Linux part?

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).

https://codereview.chromium.org/1015503002/diff/100001/base/threading/platfor...
base/threading/platform_thread_posix.cc:248: // rather than the current thread
id since they are equivalent but it makes
On 2015/03/17 22:15:22, rvargas wrote:
> is using 0 also equivalent on bsd? just checking

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

https://codereview.chromium.org/1015503002/diff/100001/base/threading/platfor...
base/threading/platform_thread_posix.cc:251: const int nice_setting =
internal::ThreadPriorityToNiceValue(priority);
On 2015/03/17 22:15:21, rvargas wrote:
> nit: either remove const (preferred) or name the variables as constant.

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.

[gab, 2015-03-18 19:04:51]

Thanks PTAL.

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

https://codereview.chromium.org/1015503002/diff/100001/base/threading/platfor...
File base/threading/platform_thread_android.cc (right):

https://codereview.chromium.org/1015503002/diff/100001/base/threading/platfor...
base/threading/platform_thread_android.cc:41: {kThreadPriority_RealtimeAudio,
-16},
On 2015/03/18 19:01:05, gab wrote:
> On 2015/03/17 22:15:21, rvargas wrote:
> > nit: spaces after { and before }
> 
> I don't feel strongly either way, but this is the output of "git cl format"
(it
> even changes it back if I explicitly add spaces).

Looks like this is the correct format these days.

https://codereview.chromium.org/1015503002/diff/100001/base/threading/platfor...
File base/threading/platform_thread_internal_posix.cc (right):

https://codereview.chromium.org/1015503002/diff/100001/base/threading/platfor...
base/threading/platform_thread_internal_posix.cc:14: for (const
ThreadPriorityToNiceValuePair& tnp :
On 2015/03/18 19:01:05, gab wrote:
> On 2015/03/17 22:15:21, rvargas wrote:
> > nit: don't abbreviate tnp 
> 
> how about "pair"? Or do you prefer something verbose list
> "priority_to_nice_value_pair"?

"pair" is fine

https://codereview.chromium.org/1015503002/diff/100001/base/threading/platfor...
File base/threading/platform_thread_posix.cc (right):

https://codereview.chromium.org/1015503002/diff/100001/base/threading/platfor...
base/threading/platform_thread_posix.cc:245: // priority. However, under the
current Linux/NPTL implementation of POSIX
On 2015/03/18 19:01:06, gab wrote:
> On 2015/03/17 22:15:21, rvargas wrote:
> > tiny nit: Remove the Linux part?
> 
> 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).

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?).

https://codereview.chromium.org/1015503002/diff/100001/base/threading/platfor...
base/threading/platform_thread_posix.cc:248: // rather than the current thread
id since they are equivalent but it makes
On 2015/03/18 19:01:06, gab wrote:
> On 2015/03/17 22:15:22, rvargas wrote:
> > is using 0 also equivalent on bsd? just checking
> 
> 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

Acknowledged.

https://codereview.chromium.org/1015503002/diff/100001/base/threading/platfor...
base/threading/platform_thread_posix.cc:251: const int nice_setting =
internal::ThreadPriorityToNiceValue(priority);
On 2015/03/18 19:01:06, gab wrote:
> On 2015/03/17 22:15:21, rvargas wrote:
> > nit: either remove const (preferred) or name the variables as constant.
> 
> 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.

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 :(

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

Tweaked comment, keeping rest as-is to balance my own pet-peeves ;-).

https://codereview.chromium.org/1015503002/diff/100001/base/threading/platfor...
File base/threading/platform_thread_posix.cc (right):

https://codereview.chromium.org/1015503002/diff/100001/base/threading/platfor...
base/threading/platform_thread_posix.cc:245: // priority. However, under the
current Linux/NPTL implementation of POSIX
On 2015/03/18 22:54:23, rvargas wrote:
> On 2015/03/18 19:01:06, gab wrote:
> > On 2015/03/17 22:15:21, rvargas wrote:
> > > tiny nit: Remove the Linux part?
> > 
> > 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).
> 
> 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?).

Fair enough, tweaked comment.

[rvargas (doing something else), 2015-03-19 17:50:45]

lgtm

[gab, 2015-03-19 17:51:16]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2015-03-19 17:51:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1015503002/220001

[commit-bot: I haz the power, 2015-03-19 19:08:30]

Committed patchset #11 (id:220001)

[commit-bot: I haz the power, 2015-03-19 19:08:56]

Patchset 11 (id:??) landed as
https://crrev.com/bb0d0b93645861e9e4dc66d31b674fdf2965ec4e
Cr-Commit-Position: refs/heads/master@{#321403}