Lazily prune the multibuffer block cache.

Lazily prune the multibuffer block cache.

Prunes ~5Mb per minute.

BUG=595827
Committed: https://crrev.com/6f09ac8211e36d634bd143881a870687bdb4c23f
Cr-Commit-Position: refs/heads/master@{#383636}

[hubbe, 2016-03-24 19:07:05]

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

[DaleCurtis, 2016-03-24 19:12:24]

Unittest?

https://codereview.chromium.org/1829163002/diff/1/media/blink/multibuffer.cc
File media/blink/multibuffer.cc (right):

https://codereview.chromium.org/1829163002/diff/1/media/blink/multibuffer.cc#...
media/blink/multibuffer.cc:115: background_pruning_active_ = false;
This disables pruning on the first scheduled one and will stop if Use/Insert is
not called. That doesn't seem like it will continue to run pruning if insert/use
is not called.

[hubbe, 2016-03-24 20:57:41]

https://codereview.chromium.org/1829163002/diff/1/media/blink/multibuffer.cc
File media/blink/multibuffer.cc (right):

https://codereview.chromium.org/1829163002/diff/1/media/blink/multibuffer.cc#...
media/blink/multibuffer.cc:115: background_pruning_active_ = false;
On 2016/03/24 19:12:24, DaleCurtis wrote:
> This disables pruning on the first scheduled one and will stop if Use/Insert
is
> not called. That doesn't seem like it will continue to run pruning if
insert/use
> is not called.

I renamed background_pruning_active_ to backround_pruning_pending_.
This doesn't disable background pruning, it just means that there no pending
call to this function anymore, which means that SchedulePrune() can schedule
another one.

[DaleCurtis, 2016-03-24 21:02:58]

When does the prune task stop running?

[hubbe, 2016-03-24 21:05:44]

On 2016/03/24 21:02:58, DaleCurtis wrote:
> When does the prune task stop running?

When there is nothing to prune.

[DaleCurtis, 2016-03-24 21:07:30]

On 2016/03/24 at 21:05:44, hubbe wrote:
> On 2016/03/24 21:02:58, DaleCurtis wrote:
> > When does the prune task stop running?
> 
> When there is nothing to prune.

[DaleCurtis, 2016-03-24 21:10:16]

On 2016/03/24 at 21:07:30, DaleCurtis wrote:
> On 2016/03/24 at 21:05:44, hubbe wrote:
> > On 2016/03/24 21:02:58, DaleCurtis wrote:
> > > When does the prune task stop running?
> > 
> > When there is nothing to prune.

Whoops, hit send accidentally. I guess this is determined by the Pruneable()
function? "data_size_ > max_size_" is an unusual statement since one doesn't
expect size to grow beyond a max. Can you explain that a bit better?

[hubbe, 2016-03-24 21:21:52]

On 2016/03/24 21:10:16, DaleCurtis wrote:
> On 2016/03/24 at 21:07:30, DaleCurtis wrote:
> > On 2016/03/24 at 21:05:44, hubbe wrote:
> > > On 2016/03/24 21:02:58, DaleCurtis wrote:
> > > > When does the prune task stop running?
> > > 
> > > When there is nothing to prune.
> 
> Whoops, hit send accidentally. I guess this is determined by the Pruneable()
> function? "data_size_ > max_size_" is an unusual statement since one doesn't
> expect size to grow beyond a max. Can you explain that a bit better?

I added some comments to multibuffer.h, let me know that clarifies things.
If not, max_size_ should probably be renamed to allowed_size_ or something.

     /Hubbe

[DaleCurtis, 2016-03-24 21:28:03]

Does max_size_ return to zero after a playback? It's not clear how the
Pruneable() function will ensure that all data is pruned after 5 minutes.

[hubbe, 2016-03-24 21:39:41]

On 2016/03/24 21:28:03, DaleCurtis wrote:
> Does max_size_ return to zero after a playback? It's not clear how the
> Pruneable() function will ensure that all data is pruned after 5 minutes.

max_size_ is decreased when the player object goes away.
As long as the player object lives, we should hold on to the cached data I
think.
(This is also what BufferedDataSource does.)

[DaleCurtis, 2016-03-24 21:47:40]

Sounds good. LG once a test is added.

[hubbe, 2016-03-25 17:41:41]

On 2016/03/24 21:47:40, DaleCurtis wrote:
> Sounds good. LG once a test is added.

As usual, test is much more involved than the code it's supposed to test.
Anyways, moving FakeSingleThreadTaskRunner to media/base should simplify testing
in the future.

[hubbe, 2016-03-25 18:07:56]

hubbe@chromium.org changed reviewers:
+ miu@chromium.org

[hubbe, 2016-03-25 18:07:57]

Adding miu@ as a media/cast FYI

[DaleCurtis, 2016-03-25 18:13:27]

Will take a look in a bit, at a glance though you might also have tested this by
just having a SetPruneTimeoutForTesting() which uses a zero-timeout and a
manually pumped message loop. See the tests I wrote for
RendererWebMediaPlayerDelegate which do something similar.

[chromium-reviews, 2016-03-25 18:20:50]

Injectable TaskRunners + Clocks is really powerful for testing though. We
did this throughout our entire code base for cast, and because of that we
could run entire end-to-end tests in fractions of a second. I highly
recommend it.


On Fri, Mar 25, 2016 at 11:13 AM, <dalecurtis@chromium.org> wrote:

> Will take a look in a bit, at a glance though you might also have tested
> this by
> just having a SetPruneTimeoutForTesting() which uses a zero-timeout and a
> manually pumped message loop. See the tests I wrote for
> RendererWebMediaPlayerDelegate which do something similar.
>
> https://codereview.chromium.org/1829163002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[miu, 2016-03-25 20:45:32]

media/base and media/cast lgtm

Code re-use FTW!  :)

[DaleCurtis, 2016-03-25 20:52:22]

lgtm should this test class be in media/test instead?

https://codereview.chromium.org/1829163002/diff/80001/media/blink/multibuffer.h
File media/blink/multibuffer.h (right):

https://codereview.chromium.org/1829163002/diff/80001/media/blink/multibuffer...
media/blink/multibuffer.h:187: scoped_refptr<base::SingleThreadTaskRunner>
task_runner_;
While you're here, DISALLOW_COPY_AND_ASSIGN() ?

[hubbe, 2016-03-25 21:06:30]

> should this test class be in media/test instead?

Possibly, but no more so than the rest of the classes in
//media/base::test_support I think.

https://codereview.chromium.org/1829163002/diff/80001/media/blink/multibuffer.h
File media/blink/multibuffer.h (right):

https://codereview.chromium.org/1829163002/diff/80001/media/blink/multibuffer...
media/blink/multibuffer.h:187: scoped_refptr<base::SingleThreadTaskRunner>
task_runner_;
On 2016/03/25 20:52:22, DaleCurtis wrote:
> While you're here, DISALLOW_COPY_AND_ASSIGN() ?

Done.

[DaleCurtis, 2016-03-25 21:37:05]

Still lgtm

[hubbe, 2016-03-28 23:38:00]

The CQ bit was checked by hubbe@chromium.org

[hubbe, 2016-03-28 23:38:01]

The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org,
dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/1829163002/#ps200001
(title: "no export")

[commit-bot: I haz the power, 2016-03-28 23:38:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1829163002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1829163002/200001

[commit-bot: I haz the power, 2016-03-29 00:46:02]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-03-29 00:47:18]

Description was changed from

==========
Lazily prune the multibuffer block cache.

Prunes ~5Mb per minute.

BUG=595827
==========

to

==========
Lazily prune the multibuffer block cache.

Prunes ~5Mb per minute.

BUG=595827

Committed: https://crrev.com/6f09ac8211e36d634bd143881a870687bdb4c23f
Cr-Commit-Position: refs/heads/master@{#383636}
==========

[commit-bot: I haz the power, 2016-03-29 00:47:19]

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