Add support for real-time audio threads.

Add support for real-time audio threads.
BUG=none
TEST=none
(tested locally on Mac OS X)
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=85663

[Chris Rogers, 2011-05-06 22:15:29]

For now, I'm leaving the Windows and Linux versions unimplemented.

On Mac OS X, I've tested this pretty extensively (both with and without the
patch) to determine audio performance when under heavy load.  This is the
technique we used at Apple to achieve good performance.

This fixes glitching problems I've been seeing as well as audio performance bug:
http://code.google.com/p/chromium/issues/detail?id=81445

[Ken Russell (switch to Gerrit), 2011-05-09 22:28:14]

http://codereview.chromium.org/6949009/diff/1/base/threading/platform_thread.h
File base/threading/platform_thread.h (right):

http://codereview.chromium.org/6949009/diff/1/base/threading/platform_thread....
base/threading/platform_thread.h:96: static void
EnableRealtimeAudioPerformance(PlatformThreadHandle handle);
An audio-specific method seems out of place in this low-level class. Can you
think of a different way to name or specify this that would have the same
effect? For example, what about passing in the time constraint parameters in
some form, which might be ignored on other operating systems, and then name this
method generically? Or what about not making the Mac implementation not quite so
audio-specific, and call it something like "BoostThreadPriority" or
"SetThreadPriorityToRealtime"?

[Chris Rogers, 2011-05-09 22:40:49]

I wanted to avoid defining a cross-platform interface where I would be passing
time constraint parameters which would not be possible to implement on the other
platforms.  With the current naming, the function can be implemented in some
fashion on all the platforms we have.

I suppose I can live with the idea of renaming the method
"SetThreadPriorityToRealtime()".  But I still prefer keeping the name as is
since, at least on the mac, the parameters are tweaked specifically for audio
and not a generic "real-time" concept.  It's not really that big of a deal
though - would changing the name to "SetThreadPriorityToRealtime()" be
sufficient for you?

[Ken Russell (switch to Gerrit), 2011-05-10 18:04:56]

On 2011/05/09 22:40:49, Chris Rogers wrote:
> I wanted to avoid defining a cross-platform interface where I would be passing
> time constraint parameters which would not be possible to implement on the
other
> platforms.  With the current naming, the function can be implemented in some
> fashion on all the platforms we have.
> 
> I suppose I can live with the idea of renaming the method
> "SetThreadPriorityToRealtime()".  But I still prefer keeping the name as is
> since, at least on the mac, the parameters are tweaked specifically for audio
> and not a generic "real-time" concept.  It's not really that big of a deal
> though - would changing the name to "SetThreadPriorityToRealtime()" be
> sufficient for you?

It would be fine with me. However cpu made a good suggestion that you add an
enum instead and make the API something like SetThreadPriority or
BoostThreadPriority taking that enum as argument. Could you make that change
instead of the one I suggested?

You'll also need an OWNERS review. Adding darin to the CC: list.

[Chris Rogers, 2011-05-10 18:46:09]

What values would you suggest for the enum?

kThreadPriority_Low
kThreadPriority_Normal
kThreadPriority_High
kThreadPriority_RealtimeAudio

I'm guessing we should probably do something similar to the Windows enums as
long as the highest one amounts to "audio priority"

[Ken Russell (switch to Gerrit), 2011-05-10 19:43:11]

On 2011/05/10 18:46:09, Chris Rogers wrote:
> What values would you suggest for the enum?
> 
> kThreadPriority_Low
> kThreadPriority_Normal
> kThreadPriority_High
> kThreadPriority_RealtimeAudio
> 
> I'm guessing we should probably do something similar to the Windows enums as
> long as the highest one amounts to "audio priority"

Sure, sounds fine. Or just add the two that you currently need (Normal /
RealtimeAudio).

[Chris Rogers, 2011-05-10 22:04:56]

I've taken your suggestions and changed this to a SetThreadPriority() method
with an enum.

[Ken Russell (switch to Gerrit), 2011-05-11 21:28:24]

LGTM

[Chris Rogers, 2011-05-11 21:42:38]

Brett could you please do an OWNERS review?

[brettw, 2011-05-12 22:26:03]

http://codereview.chromium.org/6949009/diff/9001/base/threading/platform_thre...
File base/threading/platform_thread_mac.mm (right):

http://codereview.chromium.org/6949009/diff/9001/base/threading/platform_thre...
base/threading/platform_thread_mac.mm:79: policy.timeshare = 0;  // set to 1 for
a non-fixed thread
This should be a complete sentence w/ capitals and periods.

http://codereview.chromium.org/6949009/diff/9001/base/threading/platform_thre...
base/threading/platform_thread_mac.mm:108: const double kMaxTimeAllowed = 0.85 *
kTimeQuantum;
All of these numbers look magic. Where did you get 0.75 and why multiply it by
0.85?

http://codereview.chromium.org/6949009/diff/9001/base/threading/platform_thre...
base/threading/platform_thread_mac.mm:114: double msToAbsTime =
Should should use lowercase_with_underscores.

http://codereview.chromium.org/6949009/diff/9001/base/threading/platform_thre...
File base/threading/platform_thread_posix.cc (right):

http://codereview.chromium.org/6949009/diff/9001/base/threading/platform_thre...
base/threading/platform_thread_posix.cc:230: // TODO(crogers): implement
This should use the NOTIMPLEMENTED macro.

http://codereview.chromium.org/6949009/diff/9001/base/threading/platform_thre...
File base/threading/platform_thread_win.cc (right):

http://codereview.chromium.org/6949009/diff/9001/base/threading/platform_thre...
base/threading/platform_thread_win.cc:149: // TODO(crogers): implement
Ditto

[Chris Rogers, 2011-05-12 23:01:55]

Brett, thanks for looking at this.  I'm assuming that NOTIMPLEMENTED will do
nothing worse than log a warning at runtime.

I think I've addressed all of your comments.

http://codereview.chromium.org/6949009/diff/9001/base/threading/platform_thre...
File base/threading/platform_thread_mac.mm (right):

http://codereview.chromium.org/6949009/diff/9001/base/threading/platform_thre...
base/threading/platform_thread_mac.mm:79: policy.timeshare = 0;  // set to 1 for
a non-fixed thread
On 2011/05/12 22:26:03, brettw wrote:
> This should be a complete sentence w/ capitals and periods.

Done.

http://codereview.chromium.org/6949009/diff/9001/base/threading/platform_thre...
base/threading/platform_thread_mac.mm:108: const double kMaxTimeAllowed = 0.85 *
kTimeQuantum;
On 2011/05/12 22:26:03, brettw wrote:
> All of these numbers look magic. Where did you get 0.75 and why multiply it by
> 0.85?

I've added comments and defined more constants to try to better explain these
values.

http://codereview.chromium.org/6949009/diff/9001/base/threading/platform_thre...
base/threading/platform_thread_mac.mm:114: double msToAbsTime =
On 2011/05/12 22:26:03, brettw wrote:
> Should should use lowercase_with_underscores.

Done.

http://codereview.chromium.org/6949009/diff/9001/base/threading/platform_thre...
File base/threading/platform_thread_posix.cc (right):

http://codereview.chromium.org/6949009/diff/9001/base/threading/platform_thre...
base/threading/platform_thread_posix.cc:230: // TODO(crogers): implement
On 2011/05/12 22:26:03, brettw wrote:
> This should use the NOTIMPLEMENTED macro.

Done.

http://codereview.chromium.org/6949009/diff/9001/base/threading/platform_thre...
File base/threading/platform_thread_win.cc (right):

http://codereview.chromium.org/6949009/diff/9001/base/threading/platform_thre...
base/threading/platform_thread_win.cc:149: // TODO(crogers): implement
On 2011/05/12 22:26:03, brettw wrote:
> Ditto

Done.

[brettw, 2011-05-13 05:14:22]

Thanks, LGTM.

Yes, NOTIMPLEMENTED logs a special message.

http://codereview.chromium.org/6949009/diff/15001/base/threading/platform_thr...
File base/threading/platform_thread_mac.mm (right):

http://codereview.chromium.org/6949009/diff/15001/base/threading/platform_thr...
base/threading/platform_thread_mac.mm:138: }  // anonymous namespace
Just // namespace is sufficient (don't need "anonymous").

[darin (slow to review), 2011-05-13 17:51:12]

http://codereview.chromium.org/6949009/diff/15001/base/threading/platform_thr...
File base/threading/platform_thread.h (right):

http://codereview.chromium.org/6949009/diff/15001/base/threading/platform_thr...
base/threading/platform_thread.h:54: kThreadPriority_RealtimeAudio
isn't it a bit unfortunate for base to have to know about the concept of
real-time audio???

isn't there maybe some better way to parameterize this so that a consumer could
use PlatformThread APIs to construct a thread suitable for real-time audio?

[Chris Rogers, 2011-05-13 19:23:40]

On 2011/05/13 17:51:12, darin wrote:
>
http://codereview.chromium.org/6949009/diff/15001/base/threading/platform_thr...
> File base/threading/platform_thread.h (right):
> 
>
http://codereview.chromium.org/6949009/diff/15001/base/threading/platform_thr...
> base/threading/platform_thread.h:54: kThreadPriority_RealtimeAudio
> isn't it a bit unfortunate for base to have to know about the concept of
> real-time audio???

An alternative would be to rename this to

"kThreadPriority_Realtime" or "kThreadPriority_TimeCritical"  which would remove
the word "audio".  The implementation can remain the same in this case.

We could add a few more constants similar to the Windows enum values (listed
below) if they turned out to be useful.

One reason I chose to name the constant "kThreadPriority_RealtimeAudio" instead
of something as simple as "kThreadPriority_High" is because I really wanted to
discourage a more widespread use, creating a thread arms race, etc.  With the
"audio" naming I was trying to make the use case specific and clear, so people
would think twice before using it.  That said, it's not a big deal to change the
name to something more generic.

> 
> isn't there maybe some better way to parameterize this so that a consumer
could
> use PlatformThread APIs to construct a thread suitable for real-time audio?

Unfortunately, Windows and Linux thread APIs do not offer as fine a level of
control as the mach-level thread APIs.  For example, they don't have concepts
such as "time constraints policy".  On Windows there are constants for thread
priority such as:

THREAD_PRIORITY_BELOW_NORMAL
THREAD_PRIORITY_NORMAL
THREAD_PRIORITY_ABOVE_NORMAL
THREAD_PRIORITY_TIME_CRITICAL

This is why I didn't parameterize SetThreadPriority() in any more complex way
than passing in a simple enum value.  The Windows and Linux implementations can
do something reasonable with this enum value given their OS-level APIs.

[scherkus (not reviewing), 2011-05-13 23:06:49]

http://codereview.chromium.org/6949009/diff/15001/base/threading/platform_thr...
File base/threading/platform_thread.h (right):

http://codereview.chromium.org/6949009/diff/15001/base/threading/platform_thr...
base/threading/platform_thread.h:54: kThreadPriority_RealtimeAudio
On 2011/05/13 17:51:12, darin wrote:
> isn't it a bit unfortunate for base to have to know about the concept of
> real-time audio???
> 
> isn't there maybe some better way to parameterize this so that a consumer
could
> use PlatformThread APIs to construct a thread suitable for real-time audio?
> 

I don't want to bikeshed this anymore, but what about Normal and High?

...and if setting between high/normal doesn't make sense you *could* go all the
way with just having a "IncreaseThreadPriority()" method

http://codereview.chromium.org/6949009/diff/15001/base/threading/platform_thr...
base/threading/platform_thread.h:101: static void
SetThreadPriority(PlatformThreadHandle handle,
could we get a comment + disclaimer here warning people not to carelessly bump
up the thread priority?

I think I linked you to the discussion before on chromium-dev:
http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre...

[Ken Russell (switch to Gerrit), 2011-05-13 23:11:01]

http://codereview.chromium.org/6949009/diff/15001/base/threading/platform_thr...
File base/threading/platform_thread.h (right):

http://codereview.chromium.org/6949009/diff/15001/base/threading/platform_thr...
base/threading/platform_thread.h:54: kThreadPriority_RealtimeAudio
On 2011/05/13 23:06:49, scherkus wrote:
> On 2011/05/13 17:51:12, darin wrote:
> > isn't it a bit unfortunate for base to have to know about the concept of
> > real-time audio???
> > 
> > isn't there maybe some better way to parameterize this so that a consumer
> could
> > use PlatformThread APIs to construct a thread suitable for real-time audio?
> > 
> 
> I don't want to bikeshed this anymore, but what about Normal and High?
> 
> ...and if setting between high/normal doesn't make sense you *could* go all
the
> way with just having a "IncreaseThreadPriority()" method

The Mac implementation is currently very specific to the audio use case, and I
can imagine wanting to have a high-priority enum that's different from the audio
enum. This was the motivation for suggesting the audio specific enum.

[Chris Rogers, 2011-05-13 23:43:52]

Hi Andrew,

I added a warning comment as you suggest.

On 2011/05/13 23:06:49, scherkus wrote:
>
http://codereview.chromium.org/6949009/diff/15001/base/threading/platform_thr...
> File base/threading/platform_thread.h (right):
> 
>
http://codereview.chromium.org/6949009/diff/15001/base/threading/platform_thr...
> base/threading/platform_thread.h:54: kThreadPriority_RealtimeAudio
> On 2011/05/13 17:51:12, darin wrote:
> > isn't it a bit unfortunate for base to have to know about the concept of
> > real-time audio???
> > 
> > isn't there maybe some better way to parameterize this so that a consumer
> could
> > use PlatformThread APIs to construct a thread suitable for real-time audio?
> > 
> 
> I don't want to bikeshed this anymore, but what about Normal and High?
> 
> ...and if setting between high/normal doesn't make sense you *could* go all
the
> way with just having a "IncreaseThreadPriority()" method
> 
>
http://codereview.chromium.org/6949009/diff/15001/base/threading/platform_thr...
> base/threading/platform_thread.h:101: static void
> SetThreadPriority(PlatformThreadHandle handle,
> could we get a comment + disclaimer here warning people not to carelessly bump
> up the thread priority?
> 
> I think I linked you to the discussion before on chromium-dev:
>
http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre...