Introduce AudioBufferMemoryPool to avoid thrashing on audio buffers.

Introduce AudioBufferMemoryPool to avoid thrashing on audio buffers.

We create thousands of AudioBuffer objects per minute in many cases,
local testing on Linux desktop reveals we only end up reusing about
80% of what we allocate, so add a memory pool which ensures 99%
reuse for common playback scenarios.

The AudioPlayback clockless pipeline integration test shows an
improvement in runs/s of ~4-20% depending on system load. This is
a simple change, so the gains seem worth it.

BUG=none
TEST=new AudioBuffer unittests, existing ones pass.

Review-Url: https://codereview.chromium.org/2788483003
Cr-Commit-Position: refs/heads/master@{#461308}
Committed: https://chromium.googlesource.com/chromium/src/+/6a3eacc6ccf19f2779937859de92e35a3d3dd908

[DaleCurtis, 2017-03-30 23:36:05]

dalecurtis@chromium.org changed reviewers:
+ bbudge@chromium.org, slan@chromium.org, xhwang@chromium.org

[DaleCurtis, 2017-03-30 23:36:06]

xhwang@ please review all
+bbudge for content/renderer/pepper stamp
+slan for chromecast/ stamp

[DaleCurtis, 2017-03-30 23:37:19]

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

[commit-bot: I haz the power, 2017-03-30 23:38:11]

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

[bbudge, 2017-03-30 23:42:59]

content/renderer/pepper lgtm

[commit-bot: I haz the power, 2017-03-31 00:47:57]

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

[commit-bot: I haz the power, 2017-03-31 00:47:57]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[DaleCurtis, 2017-03-31 01:20:35]

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

[commit-bot: I haz the power, 2017-03-31 01:21:23]

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

[commit-bot: I haz the power, 2017-03-31 02:20:36]

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

[commit-bot: I haz the power, 2017-03-31 02:20:37]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[DaleCurtis, 2017-03-31 03:44:40]

https://codereview.chromium.org/2788483003/diff/20001/media/base/audio_buffer.cc
File media/base/audio_buffer.cc (right):

https://codereview.chromium.org/2788483003/diff/20001/media/base/audio_buffer...
media/base/audio_buffer.cc:31: if (entries_.front().size == size)
Whoops this is wrong. Will fix tomorrow.

[slan, 2017-03-31 15:26:01]

On 2017/03/31 03:44:40, DaleCurtis wrote:
>
https://codereview.chromium.org/2788483003/diff/20001/media/base/audio_buffer.cc
> File media/base/audio_buffer.cc (right):
> 
>
https://codereview.chromium.org/2788483003/diff/20001/media/base/audio_buffer...
> media/base/audio_buffer.cc:31: if (entries_.front().size == size)
> Whoops this is wrong. Will fix tomorrow.

cast lgtm

[DaleCurtis, 2017-03-31 19:38:02]

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

[commit-bot: I haz the power, 2017-03-31 19:39:07]

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

[commit-bot: I haz the power, 2017-03-31 21:11:21]

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

[commit-bot: I haz the power, 2017-03-31 21:11:22]

Dry run: This issue passed the CQ dry run.

[xhwang, 2017-04-01 00:05:27]

nice CL! lgtm % nit

https://codereview.chromium.org/2788483003/diff/60001/media/base/audio_buffer.cc
File media/base/audio_buffer.cc (right):

https://codereview.chromium.org/2788483003/diff/60001/media/base/audio_buffer...
media/base/audio_buffer.cc:25: AudioBufferMemoryPool::AudioMemory
AudioBufferMemoryPool::CreateBuffer(
Could you please add comments summarizing the allocation logic? How often do we
expect size-mismatch (line 32)? It's also worth mentioning that the maximum
number of outstanding memory blocks should be the maximum number of concurrent
AudioBuffer so it's won't grow indefinitely.

https://codereview.chromium.org/2788483003/diff/60001/media/base/audio_buffer.h
File media/base/audio_buffer.h (right):

https://codereview.chromium.org/2788483003/diff/60001/media/base/audio_buffer...
media/base/audio_buffer.h:198: // Basic memory pool for reusing AudioBuffer
internal memory to avoid thrashing.
Please add a comment that AudioBuffer created using this pool will hold a
ref-count to it.

[DaleCurtis, 2017-04-01 01:00:00]

The CQ bit was checked by dalecurtis@chromium.org

[DaleCurtis, 2017-04-01 01:00:01]

Thanks for review! Updated class comment with buffer details.

https://codereview.chromium.org/2788483003/diff/60001/media/base/audio_buffer.cc
File media/base/audio_buffer.cc (right):

https://codereview.chromium.org/2788483003/diff/60001/media/base/audio_buffer...
media/base/audio_buffer.cc:25: AudioBufferMemoryPool::AudioMemory
AudioBufferMemoryPool::CreateBuffer(
On 2017/04/01 at 00:05:26, xhwang_slow wrote:
> Could you please add comments summarizing the allocation logic? How often do
we expect size-mismatch (line 32)? It's also worth mentioning that the maximum
number of outstanding memory blocks should be the maximum number of concurrent
AudioBuffer so it's won't grow indefinitely.

Done.

https://codereview.chromium.org/2788483003/diff/60001/media/base/audio_buffer.h
File media/base/audio_buffer.h (right):

https://codereview.chromium.org/2788483003/diff/60001/media/base/audio_buffer...
media/base/audio_buffer.h:198: // Basic memory pool for reusing AudioBuffer
internal memory to avoid thrashing.
On 2017/04/01 at 00:05:26, xhwang_slow wrote:
> Please add a comment that AudioBuffer created using this pool will hold a
ref-count to it.

Done.

[DaleCurtis, 2017-04-01 01:00:01]

The patchset sent to the CQ was uploaded after l-g-t-m from bbudge@chromium.org,
slan@chromium.org, xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/2788483003/#ps80001
(title: "Add class comments.")

[commit-bot: I haz the power, 2017-04-01 01:00:23]

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

[commit-bot: I haz the power, 2017-04-01 02:55:07]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1491008400290620,
"parent_rev": "f59d2d6a9991788922660cd731754d81545edc7a", "commit_rev":
"083d4e30738a15aafc20916598e42a7f0ae93157"}

[commit-bot: I haz the power, 2017-04-01 03:00:18]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1491008400290620,
"parent_rev": "10fc42a0f4dcdc878c3a6a54b575cd37536ebfe1", "commit_rev":
"6a3eacc6ccf19f2779937859de92e35a3d3dd908"}

[commit-bot: I haz the power, 2017-04-01 03:01:21]

Description was changed from

==========
Introduce AudioBufferMemoryPool to avoid thrashing on audio buffers.

We create thousands of AudioBuffer objects per minute in many cases,
local testing on Linux desktop reveals we only end up reusing about
80% of what we allocate, so add a memory pool which ensures 99%
reuse for common playback scenarios.

The AudioPlayback clockless pipeline integration test shows an
improvement in runs/s of ~4-20% depending on system load. This is
a simple change, so the gains seem worth it.

BUG=none
TEST=new AudioBuffer unittests, existing ones pass.
==========

to

==========
Introduce AudioBufferMemoryPool to avoid thrashing on audio buffers.

We create thousands of AudioBuffer objects per minute in many cases,
local testing on Linux desktop reveals we only end up reusing about
80% of what we allocate, so add a memory pool which ensures 99%
reuse for common playback scenarios.

The AudioPlayback clockless pipeline integration test shows an
improvement in runs/s of ~4-20% depending on system load. This is
a simple change, so the gains seem worth it.

BUG=none
TEST=new AudioBuffer unittests, existing ones pass.

Review-Url: https://codereview.chromium.org/2788483003
Cr-Commit-Position: refs/heads/master@{#461308}
Committed:
https://chromium.googlesource.com/chromium/src/+/6a3eacc6ccf19f2779937859de92...
==========

[commit-bot: I haz the power, 2017-04-01 03:01:22]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/6a3eacc6ccf19f2779937859de92...