Introduce new constructor to PlatformThread.

Introduce a new constructor to PlatformThread.

The new constructor introduces two new changes:

* Support specifying thread priority at construction time.
  - Moving forward, the SetPriority() method will be removed.
* New thread function type.
  - The new type has 'void' as a return type and a polling loop
    inside PlatformThread, is not used.

The old function type is still supported until all places have been moved over.

In this CL, the first steps towards deprecating the old mechanism are taken
by moving parts of the code that were simple to move, over to the new callback
type.

BUG=webrtc:7187
Review-Url: https://codereview.webrtc.org/2708723003
Cr-Commit-Position: refs/heads/master@{#16779}
Committed: https://chromium.googlesource.com/external/webrtc/+/0f8b403eb536ebc721201de5a272ffb66eb5ffe1

[tommi, 2017-02-21 14:10:55]

tommi@webrtc.org changed reviewers:
+ solenberg@webrtc.org

[tommi, 2017-02-21 14:11:06]

Description was changed from

==========
Introduce new constructor to PlatformThread.

The new constructor introduces two new changes:

* Support specifying thread priority at construction time.
  - Moving forward, the SetPriority() method will be removed.
* New thread function type.
  - The new type has 'void' as a return type and a polling loop
    inside PlatformThread, is not used.

The old function type is still supported until all places have been moved over.

In this CL, the first steps towards deprecating the old mechanism are taken
by moving parts of the code that were simple to move, over to the new callback
type.

BUG=webrtc:7187
==========

to

==========
Introduce a new constructor to PlatformThread.

The new constructor introduces two new changes:

* Support specifying thread priority at construction time.
  - Moving forward, the SetPriority() method will be removed.
* New thread function type.
  - The new type has 'void' as a return type and a polling loop
    inside PlatformThread, is not used.

The old function type is still supported until all places have been moved over.

In this CL, the first steps towards deprecating the old mechanism are taken
by moving parts of the code that were simple to move, over to the new callback
type.

BUG=webrtc:7187
==========

[tommi, 2017-02-21 14:11:29]

The CQ bit was checked by tommi@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-21 14:11:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-02-21 15:55:01]

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

[commit-bot: I haz the power, 2017-02-21 15:55:02]

Dry run: This issue passed the CQ dry run.

[tommi, 2017-02-21 19:01:50]

Update IncomingVideoStream

[tommi, 2017-02-21 20:08:46]

Update IncomingVideoStream again

[tommi, 2017-02-21 20:11:12]

The CQ bit was checked by tommi@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-21 20:11:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-02-21 21:10:57]

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

[commit-bot: I haz the power, 2017-02-21 21:10:58]

Dry run: This issue passed the CQ dry run.

[the sun, 2017-02-22 13:42:58]

https://codereview.webrtc.org/2708723003/diff/40001/webrtc/base/platform_thre...
File webrtc/base/platform_thread.cc (right):

https://codereview.webrtc.org/2708723003/diff/40001/webrtc/base/platform_thre...
webrtc/base/platform_thread.cc:102: RTC_DCHECK(name_.length() < 64);
Where's the limit of 64 from? To my knowledge, the Linux kernel puts a cap at 15
characters, so if we want to enforce readability in logs/profile results, I'd go
with that.

https://codereview.webrtc.org/2708723003/diff/40001/webrtc/base/platform_thre...
File webrtc/base/platform_thread.h (right):

https://codereview.webrtc.org/2708723003/diff/40001/webrtc/base/platform_thre...
webrtc/base/platform_thread.h:104: rtc::ThreadChecker worker_thread_checker_;
<bikeshed>
"worker_thread..." makes me associate it with the libjingle worker thread;
platform_thread_checker? spawned_thread_checker?
</bikeshed>

no big deal

https://codereview.webrtc.org/2708723003/diff/40001/webrtc/call/rampup_tests.cc
File webrtc/call/rampup_tests.cc (right):

https://codereview.webrtc.org/2708723003/diff/40001/webrtc/call/rampup_tests....
webrtc/call/rampup_tests.cc:303: } while (!event_.Wait(kPollIntervalMs));
It looks like RampUpTester::PerformTest() intends the thread to stop running
when it calls poller_thread_.Stop(), but AFAICT, after this change, that won't
happen until the RampUpTester dtor does event_.Set().

I must be getting it wrong since all tests pass on the try bots? Tell me how!

https://codereview.webrtc.org/2708723003/diff/40001/webrtc/call/rampup_tests....
webrtc/call/rampup_tests.cc:418: void RampUpDownUpTester::PollStats() {
oh, crikey, PollStats is overloaded...

https://codereview.webrtc.org/2708723003/diff/40001/webrtc/video/end_to_end_t...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/2708723003/diff/40001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:2186: } while (!stop_event_.Wait(1000));
note: this appears to work since stop_event_.Set() is called before
poller_thread_.Stop() in PerformTest().

[tommi, 2017-02-22 15:03:45]

Address comments and fix RampUp tests

[tommi, 2017-02-22 15:04:01]

https://codereview.webrtc.org/2708723003/diff/40001/webrtc/base/platform_thre...
File webrtc/base/platform_thread.cc (right):

https://codereview.webrtc.org/2708723003/diff/40001/webrtc/base/platform_thre...
webrtc/base/platform_thread.cc:102: RTC_DCHECK(name_.length() < 64);
On 2017/02/22 13:42:57, the sun wrote:
> Where's the limit of 64 from? To my knowledge, the Linux kernel puts a cap at
15
> characters, so if we want to enforce readability in logs/profile results, I'd
go
> with that.

15 sounds good to me. I think we break that though already in some cases. OK if
we tackle that in another CL and put down a TODO for now?

Wrt 64, it comes from Windows or rather from Windows debuggers.  The name can be
longer, but debuggers such as WinDbg copy the name into an internal buffer which
iirc is 64 chars long. If the name is 64 chars long or longer, the debugger will
actually use a non-terminated string, resulting in garbaged printouts and
interesting UI artifacts.

Enforcing some sort of a limit made sense to me, so I picked that limit. 15
works too.

https://codereview.webrtc.org/2708723003/diff/40001/webrtc/base/platform_thre...
File webrtc/base/platform_thread.h (right):

https://codereview.webrtc.org/2708723003/diff/40001/webrtc/base/platform_thre...
webrtc/base/platform_thread.h:104: rtc::ThreadChecker worker_thread_checker_;
On 2017/02/22 13:42:57, the sun wrote:
> <bikeshed>
> "worker_thread..." makes me associate it with the libjingle worker thread;
> platform_thread_checker? spawned_thread_checker?
> </bikeshed>
> 
> no big deal

Done. (went with spawned_thread_checker_)

https://codereview.webrtc.org/2708723003/diff/40001/webrtc/call/rampup_tests.cc
File webrtc/call/rampup_tests.cc (right):

https://codereview.webrtc.org/2708723003/diff/40001/webrtc/call/rampup_tests....
webrtc/call/rampup_tests.cc:303: } while (!event_.Wait(kPollIntervalMs));
On 2017/02/22 13:42:57, the sun wrote:
> It looks like RampUpTester::PerformTest() intends the thread to stop running
> when it calls poller_thread_.Stop(), but AFAICT, after this change, that won't
> happen until the RampUpTester dtor does event_.Set().
> 
> I must be getting it wrong since all tests pass on the try bots? Tell me how!

Indeed! Thanks for catching.  It turns out that these tests don't run on the
trybots, only perf bots.

The stop event is currently not being signalled in the right place, so it must
have always been timing out.  Now fixed. Btw, I fixed similar code yesterday, so
maybe this pattern has been copied to more places:
https://codereview.webrtc.org/2706223002/

https://codereview.webrtc.org/2708723003/diff/40001/webrtc/call/rampup_tests....
webrtc/call/rampup_tests.cc:418: void RampUpDownUpTester::PollStats() {
On 2017/02/22 13:42:58, the sun wrote:
> oh, crikey, PollStats is overloaded...

Acknowledged.

https://codereview.webrtc.org/2708723003/diff/40001/webrtc/video/end_to_end_t...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/2708723003/diff/40001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:2186: } while (!stop_event_.Wait(1000));
On 2017/02/22 13:42:58, the sun wrote:
> note: this appears to work since stop_event_.Set() is called before
> poller_thread_.Stop() in PerformTest().

Yes, that's the case I found and fixed yesterday.

[tommi, 2017-02-22 15:05:00]

The CQ bit was checked by tommi@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-22 15:05:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-02-22 15:21:28]

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

[commit-bot: I haz the power, 2017-02-22 15:21:29]

Dry run: Try jobs failed on following builders:
  mac_baremetal on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/19076)

[tommi, 2017-02-22 16:14:27]

Disable RTC_DCHECK in channel_proxy + add TODO

[tommi, 2017-02-22 16:15:50]

On 2017/02/22 16:14:27, tommi (webrtc) wrote:
> Disable RTC_DCHECK in channel_proxy + add TODO

solenberg - please take a look at the change in channel_proxy. I believe the
failures on the Mac and Android bots are related to this cl:
https://codereview.webrtc.org/2689183002/
I sent the call stack to you over email.

[tommi, 2017-02-22 16:15:56]

The CQ bit was checked by tommi@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-22 16:16:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[the sun, 2017-02-22 16:31:30]

lgtm

https://codereview.webrtc.org/2708723003/diff/40001/webrtc/base/platform_thre...
File webrtc/base/platform_thread.cc (right):

https://codereview.webrtc.org/2708723003/diff/40001/webrtc/base/platform_thre...
webrtc/base/platform_thread.cc:102: RTC_DCHECK(name_.length() < 64);
On 2017/02/22 15:04:00, tommi (webrtc) wrote:
> On 2017/02/22 13:42:57, the sun wrote:
> > Where's the limit of 64 from? To my knowledge, the Linux kernel puts a cap
at
> 15
> > characters, so if we want to enforce readability in logs/profile results,
I'd
> go
> > with that.
> 
> 15 sounds good to me. I think we break that though already in some cases. OK
if
> we tackle that in another CL and put down a TODO for now?
> 
> Wrt 64, it comes from Windows or rather from Windows debuggers.  The name can
be
> longer, but debuggers such as WinDbg copy the name into an internal buffer
which
> iirc is 64 chars long. If the name is 64 chars long or longer, the debugger
will
> actually use a non-terminated string, resulting in garbaged printouts and
> interesting UI artifacts.
> 
> Enforcing some sort of a limit made sense to me, so I picked that limit. 15
> works too.

sgtm

https://codereview.webrtc.org/2708723003/diff/40001/webrtc/call/rampup_tests.cc
File webrtc/call/rampup_tests.cc (right):

https://codereview.webrtc.org/2708723003/diff/40001/webrtc/call/rampup_tests....
webrtc/call/rampup_tests.cc:303: } while (!event_.Wait(kPollIntervalMs));
On 2017/02/22 15:04:01, tommi (webrtc) wrote:
> On 2017/02/22 13:42:57, the sun wrote:
> > It looks like RampUpTester::PerformTest() intends the thread to stop running
> > when it calls poller_thread_.Stop(), but AFAICT, after this change, that
won't
> > happen until the RampUpTester dtor does event_.Set().
> > 
> > I must be getting it wrong since all tests pass on the try bots? Tell me
how!
> 
> Indeed! Thanks for catching.  It turns out that these tests don't run on the
> trybots, only perf bots.
> 
> The stop event is currently not being signalled in the right place, so it must
> have always been timing out.  Now fixed. Btw, I fixed similar code yesterday,
so
> maybe this pattern has been copied to more places:
> https://codereview.webrtc.org/2706223002/

I looked at the other places in your CL, and they appear to do it correctly.
That's the beauty of copy+paste - it will be *almost* the same. :)

[commit-bot: I haz the power, 2017-02-22 16:42:44]

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

[commit-bot: I haz the power, 2017-02-22 16:42:44]

Dry run: Try jobs failed on following builders:
  android_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/21659)

[tommi, 2017-02-22 19:03:22]

The CQ bit was checked by tommi@webrtc.org

[commit-bot: I haz the power, 2017-02-22 19:03:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-02-22 19:22:06]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1487790202088090,
"parent_rev": "6d230d7b1dcc31c6b7d57b50a2c6379f7f3e112d", "commit_rev":
"0f8b403eb536ebc721201de5a272ffb66eb5ffe1"}

[commit-bot: I haz the power, 2017-02-22 19:22:09]

Description was changed from

==========
Introduce a new constructor to PlatformThread.

The new constructor introduces two new changes:

* Support specifying thread priority at construction time.
  - Moving forward, the SetPriority() method will be removed.
* New thread function type.
  - The new type has 'void' as a return type and a polling loop
    inside PlatformThread, is not used.

The old function type is still supported until all places have been moved over.

In this CL, the first steps towards deprecating the old mechanism are taken
by moving parts of the code that were simple to move, over to the new callback
type.

BUG=webrtc:7187
==========

to

==========
Introduce a new constructor to PlatformThread.

The new constructor introduces two new changes:

* Support specifying thread priority at construction time.
  - Moving forward, the SetPriority() method will be removed.
* New thread function type.
  - The new type has 'void' as a return type and a polling loop
    inside PlatformThread, is not used.

The old function type is still supported until all places have been moved over.

In this CL, the first steps towards deprecating the old mechanism are taken
by moving parts of the code that were simple to move, over to the new callback
type.

BUG=webrtc:7187

Review-Url: https://codereview.webrtc.org/2708723003
Cr-Commit-Position: refs/heads/master@{#16779}
Committed:
https://chromium.googlesource.com/external/webrtc/+/0f8b403eb536ebc721201de5a...
==========

[commit-bot: I haz the power, 2017-02-22 19:22:11]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/external/webrtc/+/0f8b403eb536ebc721201de5a...