base: Set initial thread priority in ThreadFunc.

base: Set initial thread priority in ThreadFunc.

Instead of relying on base::InitOnThread to set the initial thread
priority to normal, always set it to thread_params->priority in
ThreadFunc.

The result is that thread priority will be set correctly on all
posix platforms and not only Android.

BUG=584025
TEST=base_unittests --gtest_filter=PlatformThreadTest.ThreadPriorityCurrentThread

Committed: https://crrev.com/45828eb26e18f1ba333f0c39b20036a60932886b
Cr-Commit-Position: refs/heads/master@{#373761}

[reveman, 2016-02-03 23:16:13]

reveman@chromium.org changed reviewers:
+ thakis@chromium.org

[Robert Sesek, 2016-02-03 23:23:19]

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

[Robert Sesek, 2016-02-03 23:23:19]

https://codereview.chromium.org/1660273004/diff/1/base/threading/platform_thr...
File base/threading/platform_thread_android.cc (right):

https://codereview.chromium.org/1660273004/diff/1/base/threading/platform_thr...
base/threading/platform_thread_android.cc:87: void InitOnThread() {
I think you can just remove base::InitOnThread now (all other implementations
other than Android are empty).

[reveman, 2016-02-03 23:31:03]

https://codereview.chromium.org/1660273004/diff/1/base/threading/platform_thr...
File base/threading/platform_thread_android.cc (right):

https://codereview.chromium.org/1660273004/diff/1/base/threading/platform_thr...
base/threading/platform_thread_android.cc:87: void InitOnThread() {
On 2016/02/03 at 23:23:19, Robert Sesek wrote:
> I think you can just remove base::InitOnThread now (all other implementations
other than Android are empty).

Done.

[Nico, 2016-02-03 23:31:24]

thakis@chromium.org changed reviewers:
+ epenner@chromium.org, toyoshim@chromium.org

[Nico, 2016-02-03 23:31:24]

Looks ok. +toyoshim who worked with this code not too long ago, and +epenner who
wrote this years ago in case they remember any pitfalls here. If they don't
reply within a day, lgtm (with rsesek's comment addressed).

[reveman, 2016-02-03 23:54:12]

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

[commit-bot: I haz the power, 2016-02-03 23:54:44]

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

[Robert Sesek, 2016-02-04 00:10:00]

lgtm

[commit-bot: I haz the power, 2016-02-04 00:36:13]

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

[commit-bot: I haz the power, 2016-02-04 00:36:14]

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

[Takashi Toyoshima, 2016-02-04 04:48:19]

looks nice clean up, but the test failures seem like real failures.
Some sandbox configurations do not allow to change the thread priority, and it
is caught by the sandbox.

[reveman, 2016-02-04 12:53:09]

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

[commit-bot: I haz the power, 2016-02-04 12:53:25]

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

[commit-bot: I haz the power, 2016-02-04 13:08:32]

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

[commit-bot: I haz the power, 2016-02-04 13:08:32]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[reveman, 2016-02-04 14:23:25]

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

[commit-bot: I haz the power, 2016-02-04 14:23:31]

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

[reveman, 2016-02-04 14:41:56]

PTAL.

I had to remove the pthread_getschedparam call done each time we try to set the
priority on linux as that's not allowed in the sandbox. The result is that once
we've changed to realtime priority we can't go back. Not ideal but we have the
same limitation on other platforms so now we're at least broken in a consistent
way.

Note that I also had to also adjust the unit test to not fail because of this.

[reveman, 2016-02-04 14:42:53]

On 2016/02/04 at 04:48:19, toyoshim wrote:
> looks nice clean up, but the test failures seem like real failures.
> Some sandbox configurations do not allow to change the thread priority, and it
is caught by the sandbox.

I think latest patch should fix that issue.

[reveman, 2016-02-04 15:02:02]

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

[commit-bot: I haz the power, 2016-02-04 15:03:17]

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

[commit-bot: I haz the power, 2016-02-04 16:27:37]

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

[commit-bot: I haz the power, 2016-02-04 16:27:38]

Dry run: This issue passed the CQ dry run.

[reveman, 2016-02-04 22:51:36]

I'd like to land this. thakis, does the latest patch look ok to you?

[Nico, 2016-02-04 22:58:23]

I'd like toyoshim to ok effectively the revert of
https://codereview.chromium.org/1205703005 (sounds like this was for a bug that
got wontfix'd in the end, so I'm guessing it'll be ok)

If he's cool with this, then lgtm

[reveman, 2016-02-04 23:35:57]

toyoshim, does the latest patch look ok? I fixed the sandbox problems by
removing the code that was checking the current priority each time we set
priority on linux.

[Takashi Toyoshima, 2016-02-05 04:38:03]

Description was changed from

==========
base: Set initial thread priority in ThreadFunc.

Instead of relying on base::InitOnThread to set the initial thread
priority to normal, always set it to thread_params->priority in
ThreadFunc.

The result is that thread priority will be set correctly on all
posix platforms and not only Android.

BUG=584025
TEST=base_unittests
--gtest_filter=PlatformThreadTest.ThreadPriorityCurrentThread
==========

to

==========
base: Set initial thread priority in ThreadFunc.

Instead of relying on base::InitOnThread to set the initial thread
priority to normal, always set it to thread_params->priority in
ThreadFunc.

The result is that thread priority will be set correctly on all
posix platforms and not only Android.

BUG=584025
TEST=base_unittests
--gtest_filter=PlatformThreadTest.ThreadPriorityCurrentThread
==========

[Takashi Toyoshima, 2016-02-05 04:39:55]

I just found and fixed the issue in the process of my work. And there wasn't a
real use case as far as I checked when I made the original change.
Still src/media/audio/audio_device_thread.cc is the only user of REALTIME_AUDIO,
and it doesn't change its thread priority.

LGTM, but I added dalecurtis@chromium.org from media/OWNERS to CC just in case.

[Takashi Toyoshima, 2016-02-05 04:46:14]

> The result is that once we've changed to realtime priority we can't go back
platform_thread.h has some comments about SetCurrentThreadPriority()'s
restriction.
It contains some platform specific restriction. So, probably this should be
commented there together.

[reveman, 2016-02-05 05:22:08]

On 2016/02/05 at 04:46:14, toyoshim wrote:
> > The result is that once we've changed to realtime priority we can't go back
> platform_thread.h has some comments about SetCurrentThreadPriority()'s
restriction.
> It contains some platform specific restriction. So, probably this should be
commented there together.

Done.

[reveman, 2016-02-05 05:22:44]

The CQ bit was checked by reveman@chromium.org

[reveman, 2016-02-05 05:22:44]

The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org,
toyoshim@chromium.org, thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1660273004/#ps100001
(title: "cl format and add comment")

[commit-bot: I haz the power, 2016-02-05 05:22:53]

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

[commit-bot: I haz the power, 2016-02-05 07:08:08]

Description was changed from

==========
base: Set initial thread priority in ThreadFunc.

Instead of relying on base::InitOnThread to set the initial thread
priority to normal, always set it to thread_params->priority in
ThreadFunc.

The result is that thread priority will be set correctly on all
posix platforms and not only Android.

BUG=584025
TEST=base_unittests
--gtest_filter=PlatformThreadTest.ThreadPriorityCurrentThread
==========

to

==========
base: Set initial thread priority in ThreadFunc.

Instead of relying on base::InitOnThread to set the initial thread
priority to normal, always set it to thread_params->priority in
ThreadFunc.

The result is that thread priority will be set correctly on all
posix platforms and not only Android.

BUG=584025
TEST=base_unittests
--gtest_filter=PlatformThreadTest.ThreadPriorityCurrentThread
==========

[commit-bot: I haz the power, 2016-02-05 07:08:09]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-02-05 07:09:01]

Description was changed from

==========
base: Set initial thread priority in ThreadFunc.

Instead of relying on base::InitOnThread to set the initial thread
priority to normal, always set it to thread_params->priority in
ThreadFunc.

The result is that thread priority will be set correctly on all
posix platforms and not only Android.

BUG=584025
TEST=base_unittests
--gtest_filter=PlatformThreadTest.ThreadPriorityCurrentThread
==========

to

==========
base: Set initial thread priority in ThreadFunc.

Instead of relying on base::InitOnThread to set the initial thread
priority to normal, always set it to thread_params->priority in
ThreadFunc.

The result is that thread priority will be set correctly on all
posix platforms and not only Android.

BUG=584025
TEST=base_unittests
--gtest_filter=PlatformThreadTest.ThreadPriorityCurrentThread

Committed: https://crrev.com/45828eb26e18f1ba333f0c39b20036a60932886b
Cr-Commit-Position: refs/heads/master@{#373761}
==========

[commit-bot: I haz the power, 2016-02-05 07:09:02]

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

[DaleCurtis, 2016-02-05 18:11:51]

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

[DaleCurtis, 2016-02-05 18:11:51]

Thanks for the cc, this should be fine. I don't think we reduce priority
anywhere from REALTIME_AUDIO (though maybe we should have when the thread is
idle). lgtm