Abort expensive AudioManager methods once shutdown starts.

Abort expensive AudioManager methods once shutdown starts.

Crash reports show streams being created after shutdown starts, which
is wasteful behavior and should be avoided.

Shutdown is tracked via a new flag which can be checked on the audio
thread to early exit stream creation early.

BUG=none
TEST=none

[DaleCurtis, 2014-02-12 23:12:00]

WDYT?

[scherkus (not reviewing), 2014-02-26 04:55:10]

found this in the depths of my inbox ... does this still need to be reviewed?

[DaleCurtis, 2014-02-26 18:59:55]

yup, should speed up shutdown by up to a few hundred ms when there's pending
stream creation.

[scherkus (not reviewing), 2014-02-27 03:56:22]

On 2014/02/26 18:59:55, DaleCurtis wrote:
> yup, should speed up shutdown by up to a few hundred ms when there's pending
> stream creation.

OOC do we have shutdown times / know how bad of a problem this is?

Also do we know if these stream creation tasks are:
  1) existing queued tasks running before AMB::ShutdownOnIOThread() runs
  2) new queued tasks running after AMB::ShutdownOnIOThread() runs (i.e.,
preventing the MessageLoop from going idle so it can quit)

AFAICT this CL only protects against (2), which is a bit scary considering we're
in the process of deleting AMB (!)

[DaleCurtis, 2014-02-27 19:13:31]

Ah good catch, yes the |is_shutdown_| flag should be set atomically in
Shutdown() if we want to do this.  Not where it is currently.

I only have UMA statistics showing the average time of Create/Close operations
and crash reports showing we have these operations in flight at time of
Shutdown.

Just 1) since AMB isn't destructed until BrowserMainLoop is shutting down.  As
the renderer IPC channels close (which should all be before BML starts shutting
down) they'll queue Close() operations via AudioRendererHost.  So it's possible
there is a sequence of Create() -> Close() queued on the audio thread.

Since this will require atomics, I can add a ScopedHistogramTimer to
AMB::Shutdown() to measure if this is a problem first.  I'll hold off on this CL
until we have that data.

[scherkus (not reviewing), 2014-02-27 19:23:37]

On 2014/02/27 19:13:31, DaleCurtis wrote:
> Ah good catch, yes the |is_shutdown_| flag should be set atomically in
> Shutdown() if we want to do this.  Not where it is currently.
> 
> I only have UMA statistics showing the average time of Create/Close operations
> and crash reports showing we have these operations in flight at time of
> Shutdown.
> 
> Just 1) since AMB isn't destructed until BrowserMainLoop is shutting down.  As
> the renderer IPC channels close (which should all be before BML starts
shutting
> down) they'll queue Close() operations via AudioRendererHost.  So it's
possible
> there is a sequence of Create() -> Close() queued on the audio thread.
> 
> Since this will require atomics, I can add a ScopedHistogramTimer to
> AMB::Shutdown() to measure if this is a problem first.  I'll hold off on this
CL
> until we have that data.

SGTM -- keep in mind each AMB subclass does a bit of extra work in their dtor.
It'd be easiest to measure this in ~BrowserMainLoop by an explicit call to
audio_manager_.reset() but I'll leave it up to you.

It'd also be worth finding someone who is knowledgeable / cares about shutdown
performance (playmobil@, maybe?) to see if this is something worth investigating
and whether we have existing stats.

[DaleCurtis, 2014-03-01 00:37:13]

Sadly UMA stats are shutdown before we get to AudioManager destruction.  It's
also not easy to move AudioManager shutdown earlier given all its dependents. :/

I've uploaded the atomics version for posterity, but if no one cares about this
it may not be worth landing.

[scherkus (not reviewing), 2014-03-01 00:45:26]

On 2014/03/01 00:37:13, DaleCurtis wrote:
> Sadly UMA stats are shutdown before we get to AudioManager destruction.  It's
> also not easy to move AudioManager shutdown earlier given all its dependents.
:/
> 
> I've uploaded the atomics version for posterity, but if no one cares about
this
> it may not be worth landing.

Yeah TBH I'm not losing a lot of sleep over this one. If anything we could try
good old fashioned local repro + benchmarking on, say, a release window build
with LOG()'d times.

[DaleCurtis, 2014-03-04 22:25:50]

Closing then.