Avoid flooding main thread by caching state on media thread.

Avoid flooding main thread by caching state on media thread.

OnStatisticsUpdate and OnBufferedTimeRangesChanged fire much more
frequently than queried for. Posting all state change notifications
to the main thread causes potential performance issues. Cache state
not immediately required by PipelineClient on the media thread to
avoid having to post all state change notifications.

BUG=619975
Committed: https://crrev.com/8f268bb64214eab73f00216b96b919a1b2955999
Cr-Commit-Position: refs/heads/master@{#401457}

[alokp, 2016-06-16 04:31:18]

alokp@chromium.org changed reviewers:
+ sandersd@chromium.org

[alokp, 2016-06-16 04:31:19]

Perf tryjob is here: https://codereview.chromium.org/2073613002/

[DaleCurtis, 2016-06-16 16:33:57]

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

[DaleCurtis, 2016-06-16 16:33:57]

I suspect this is now duplicating the logic in RendererMediaLog for similar
reasons.

[alokp, 2016-06-16 16:42:18]

On 2016/06/16 16:33:57, DaleCurtis wrote:
> I suspect this is now duplicating the logic in RendererMediaLog for similar
> reasons.

Do you mean this? https://cs.chromium.org/chromium/src/media/base/media_log.h

AFAICT this patch only moves some variables from PipelineImpl to
RendererWrapper. Can you point where exactly you see duplication of logic?

[DaleCurtis, 2016-06-16 17:06:24]

I mean this
https://cs.chromium.org/chromium/src/content/renderer/media/render_media_log....

[DaleCurtis, 2016-06-16 17:06:36]

Probably the real fix is to stop sending so many of these at the source.

[alokp, 2016-06-16 18:01:07]

On 2016/06/16 17:06:36, DaleCurtis wrote:
> Probably the real fix is to stop sending so many of these at the source.

+mlamouri

Yeah I would prefer that solution as well. It will be hard to match the
frequency of state update notifications to the frequency at which state is
queried for though. It will be a trade-off between performance and freshness of
state but perhaps it will OK for some states depending on usage.

Pipeline::GetStatistics():
Used to report prefixed media element attributes (webkitDecodedFrameCount,
webkitDroppedFrameCount, webkitAudioDecodedByteCount,
webkitVideoDecodedByteCount) and WMPI::ReportMemoryUsage. I do not know how long
we plan to support the prefixed attributes. ReportMemoryUsage runs on a timer,
so it will be quite easy to directly plumb OnStatisticsUpdate to
ReportMemoryUsage.

The calls to Pipeline::DidLoadingProgress and Pipeline::GetBufferedTimeRanges
originate in blink HTMLMediaElement that I am not too familiar with. If there is
a prescribed time interval for these events to fire, we can define these in
PipelineImpl and change all media::Renderer implementations to respect it.

[alokp, 2016-06-16 18:01:25]

Description was changed from

==========
Avoid flooding main thread by caching state on media thread.

OnStatisticsUpdate and OnBufferedTimeRangesChanged fire much more
frequently than queried for. Posting all state change notifications
to the main thread causes potential performance issues. Cache state
not immediately required by PipelineClient on the media thread to
avoid having to post all state change notifications.

BUG=619975
==========

to

==========
Avoid flooding main thread by caching state on media thread.

OnStatisticsUpdate and OnBufferedTimeRangesChanged fire much more
frequently than queried for. Posting all state change notifications
to the main thread causes potential performance issues. Cache state
not immediately required by PipelineClient on the media thread to
avoid having to post all state change notifications.

BUG=619975
==========

[alokp, 2016-06-17 22:47:13]

Attempt to reduce the number of OnStatisticsUpdate notifications at source is
here: https://codereview.chromium.org/2079923002/

I am still waiting for the perf tryrun to see if it makes any difference.

[DaleCurtis, 2016-06-22 15:21:15]

Did you perf try run come through and show this fixed everything?

https://codereview.chromium.org/2071503003/diff/1/media/base/pipeline_impl.cc
File media/base/pipeline_impl.cc (right):

https://codereview.chromium.org/2071503003/diff/1/media/base/pipeline_impl.cc...
media/base/pipeline_impl.cc:72: SharedState()
Just use = initialization.

https://codereview.chromium.org/2071503003/diff/1/media/base/pipeline_impl.cc...
media/base/pipeline_impl.cc:242: weak_factory_.InvalidateWeakPtrs();
Should be okay, we call Stop() and destruction on the same thread IIRC. Why did
you move this? I think this should go after the else clause (where it is
currently) otherwise the posted task will be triggered before.

https://codereview.chromium.org/2071503003/diff/1/media/base/pipeline_impl.cc...
media/base/pipeline_impl.cc:1059: base::AutoLock auto_lock(shared_state_lock_);
Weird scoping isn't necessary anymore. You can just use lock + renderer.reset().

[alokp, 2016-06-22 16:27:54]

Yes it does fix perf regressions:
http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-06...

The new regressions are for seek times that too small to begin with (< 3ms). So
they may be just noise.

https://codereview.chromium.org/2071503003/diff/1/media/base/pipeline_impl.cc
File media/base/pipeline_impl.cc (right):

https://codereview.chromium.org/2071503003/diff/1/media/base/pipeline_impl.cc...
media/base/pipeline_impl.cc:72: SharedState()
On 2016/06/22 15:21:15, DaleCurtis wrote:
> Just use = initialization.

Done.

https://codereview.chromium.org/2071503003/diff/1/media/base/pipeline_impl.cc...
media/base/pipeline_impl.cc:242: weak_factory_.InvalidateWeakPtrs();
On 2016/06/22 15:21:15, DaleCurtis wrote:
> Should be okay, we call Stop() and destruction on the same thread IIRC. Why
did
> you move this? I think this should go after the else clause (where it is
> currently) otherwise the posted task will be triggered before.

It does not matter because we use WaitableEvent here. I find it cleaner to
shutdown all notifications from the media thread before posting the stop task to
the media thread (note that stop_cb is not posted back to the main thread; it
gets Run on the media thread).

I did not understand your "otherwise the posted task will be triggered before"
comment. Which posted task did you mean?

https://codereview.chromium.org/2071503003/diff/1/media/base/pipeline_impl.cc...
media/base/pipeline_impl.cc:1059: base::AutoLock auto_lock(shared_state_lock_);
On 2016/06/22 15:21:15, DaleCurtis wrote:
> Weird scoping isn't necessary anymore. You can just use lock +
renderer.reset().

If Renderer destructor is expensive, wouldn't we hold the lock for a long time
unnecessarily?

[DaleCurtis, 2016-06-22 20:40:39]

https://codereview.chromium.org/2071503003/diff/1/media/base/pipeline_impl.cc
File media/base/pipeline_impl.cc (right):

https://codereview.chromium.org/2071503003/diff/1/media/base/pipeline_impl.cc...
media/base/pipeline_impl.cc:242: weak_factory_.InvalidateWeakPtrs();
On 2016/06/22 at 16:27:54, alokp wrote:
> On 2016/06/22 15:21:15, DaleCurtis wrote:
> > Should be okay, we call Stop() and destruction on the same thread IIRC. Why
did
> > you move this? I think this should go after the else clause (where it is
> > currently) otherwise the posted task will be triggered before.
> 
> It does not matter because we use WaitableEvent here. I find it cleaner to
shutdown all notifications from the media thread before posting the stop task to
the media thread (note that stop_cb is not posted back to the main thread; it
gets Run on the media thread).
> 
> I did not understand your "otherwise the posted task will be triggered before"
comment. Which posted task did you mean?

I just meant that you're now invalidating WeakPtrs to tasks which may not have
completed yet. You're also resetting these values before stop completes. This
seems like a risky change.

[alokp, 2016-06-22 20:52:03]

https://codereview.chromium.org/2071503003/diff/1/media/base/pipeline_impl.cc
File media/base/pipeline_impl.cc (right):

https://codereview.chromium.org/2071503003/diff/1/media/base/pipeline_impl.cc...
media/base/pipeline_impl.cc:242: weak_factory_.InvalidateWeakPtrs();
On 2016/06/22 20:40:39, DaleCurtis wrote:
> On 2016/06/22 at 16:27:54, alokp wrote:
> > On 2016/06/22 15:21:15, DaleCurtis wrote:
> > > Should be okay, we call Stop() and destruction on the same thread IIRC.
Why
> did
> > > you move this? I think this should go after the else clause (where it is
> > > currently) otherwise the posted task will be triggered before.
> > 
> > It does not matter because we use WaitableEvent here. I find it cleaner to
> shutdown all notifications from the media thread before posting the stop task
to
> the media thread (note that stop_cb is not posted back to the main thread; it
> gets Run on the media thread).
> > 
> > I did not understand your "otherwise the posted task will be triggered
before"
> comment. Which posted task did you mean?
> 
> I just meant that you're now invalidating WeakPtrs to tasks which may not have
> completed yet. You're also resetting these values before stop completes. This
> seems like a risky change.

And those tasks would not complete anyways. We start waiting here after posting
the stop task during which time the main thread will be suspended and none of
the tasks posted on the main thread will run. After stop task completes, we
reset the WeakPtr and callbacks. So it is functionally equivalent (except for
unittests where we do not wait).

That said this change is not needed. I will revert.

[DaleCurtis, 2016-06-22 21:00:07]

lgtm % one additional revert.

https://codereview.chromium.org/2071503003/diff/60001/media/base/pipeline_imp...
File media/base/pipeline_impl.cc (right):

https://codereview.chromium.org/2071503003/diff/60001/media/base/pipeline_imp...
media/base/pipeline_impl.cc:272: // Invalidate self weak pointers effectively
canceling all pending
This might break if we have any unittests not calling Stop() since we use
DeleteSoon() on the media thread above, I think this part should be reverted as
well.

https://codereview.chromium.org/2071503003/diff/60001/media/base/pipeline_imp...
media/base/pipeline_impl.cc:1055: std::unique_ptr<Renderer> renderer;
Add a comment here then if you think it matters how long we hold the lock. I
don't think it does, but fine with keeping it as is.

[alokp, 2016-06-22 21:13:34]

https://codereview.chromium.org/2071503003/diff/60001/media/base/pipeline_imp...
File media/base/pipeline_impl.cc (right):

https://codereview.chromium.org/2071503003/diff/60001/media/base/pipeline_imp...
media/base/pipeline_impl.cc:272: // Invalidate self weak pointers effectively
canceling all pending
On 2016/06/22 21:00:06, DaleCurtis wrote:
> This might break if we have any unittests not calling Stop() since we use
> DeleteSoon() on the media thread above, I think this part should be reverted
as
> well.

Doing this allows us to not have to check for IsRunning in various callbacks
from media thread. Note the "if (!IsRunning())" changes to
"DCHECK(IsRunning())", which is nice IMO. We could not do this before because
the state was being cached in PipelineImpl and some unittests relied on the fact
that some state-change notifications get triggered even after Stop.

All PipelineImpl unittests call Stop, otherwise they will DCHECK in PipelineImpl
destructor:

https://cs.chromium.org/chromium/src/media/base/pipeline_impl_unittest.cc?q=p...

https://cs.chromium.org/chromium/src/media/test/pipeline_integration_test_bas...

Are you concerned about any other test?

https://codereview.chromium.org/2071503003/diff/60001/media/base/pipeline_imp...
media/base/pipeline_impl.cc:1055: std::unique_ptr<Renderer> renderer;
On 2016/06/22 21:00:07, DaleCurtis wrote:
> Add a comment here then if you think it matters how long we hold the lock. I
> don't think it does, but fine with keeping it as is.

Done.

[DaleCurtis, 2016-06-22 21:22:19]

Okay, then lets just add a DCHECK(!weak_factory_.HasWeakPtrs()); in the
destructor.

[alokp, 2016-06-22 21:42:11]

On 2016/06/22 21:22:19, DaleCurtis wrote:
> Okay, then lets just add a DCHECK(!weak_factory_.HasWeakPtrs()); in the
> destructor.

DONE. Although I needed to move the creation of WeakPtr<PipelineImpl> to
PipelineImpl::Start so that tests that do not even start the pipeline do not hit
DCHECK(!weak_factory_.HasWeakPtrs()). I think this is a good change. Now the
weakptr is only valid between Start and Stop.

[DaleCurtis, 2016-06-22 21:45:54]

lgtm

[alokp, 2016-06-22 21:48:41]

The CQ bit was checked by alokp@chromium.org

[commit-bot: I haz the power, 2016-06-22 21:49:14]

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

[commit-bot: I haz the power, 2016-06-22 23:27:05]

Description was changed from

==========
Avoid flooding main thread by caching state on media thread.

OnStatisticsUpdate and OnBufferedTimeRangesChanged fire much more
frequently than queried for. Posting all state change notifications
to the main thread causes potential performance issues. Cache state
not immediately required by PipelineClient on the media thread to
avoid having to post all state change notifications.

BUG=619975
==========

to

==========
Avoid flooding main thread by caching state on media thread.

OnStatisticsUpdate and OnBufferedTimeRangesChanged fire much more
frequently than queried for. Posting all state change notifications
to the main thread causes potential performance issues. Cache state
not immediately required by PipelineClient on the media thread to
avoid having to post all state change notifications.

BUG=619975
==========

[commit-bot: I haz the power, 2016-06-22 23:27:06]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-06-22 23:30:23]

Description was changed from

==========
Avoid flooding main thread by caching state on media thread.

OnStatisticsUpdate and OnBufferedTimeRangesChanged fire much more
frequently than queried for. Posting all state change notifications
to the main thread causes potential performance issues. Cache state
not immediately required by PipelineClient on the media thread to
avoid having to post all state change notifications.

BUG=619975
==========

to

==========
Avoid flooding main thread by caching state on media thread.

OnStatisticsUpdate and OnBufferedTimeRangesChanged fire much more
frequently than queried for. Posting all state change notifications
to the main thread causes potential performance issues. Cache state
not immediately required by PipelineClient on the media thread to
avoid having to post all state change notifications.

BUG=619975

Committed: https://crrev.com/8f268bb64214eab73f00216b96b919a1b2955999
Cr-Commit-Position: refs/heads/master@{#401457}
==========

[commit-bot: I haz the power, 2016-06-22 23:30:24]

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