585864 - Increase media cache block size from 4kB to 32kB

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Matthew Gregan [:kinetik]]

Bug 572235 increased the default media cache size from 50MB to 500MB.  We should also increase the media block size from 4kB to 32kB.

Doing this will reduce the amount of time spent performing per-block housekeeping.  With a 4kB block size and 50MB cache, a full cache would contain 12800 blocks.  With the current 500MB cache, that grows to 128000 blocks.  Moving to a 32kB block size reduces this to 16000 blocks.

The only drawback of this change would be that it takes slightly longer to drop unused data from the cache because the granularity of cached data is coarser.

Looking at a specific example, the Prince of Persia film trailer on YouTube is 24 FPS, 11.7MB and 2m31s, which means the average bitrate is ~80kB/s.  A 4kB block would contain ~50ms (~1.2 frames) of data and a 32kB block would contain ~400ms (~9.6 frames).

I'd post the (trivial) patch, but I'm seeing timeouts in test_mozLoadFrom and test_progress with an increased block size, so this needs more investigation.

Comment 1

[Matthew Gregan [:kinetik]]

Attachment: patch v0

Trivial version of the patch.  This includes two fixes for random orange that we hit with a larger block size and may also occur with the current block size (less frequently):

- Queue a cache update when a stream is opened.  This is necessary for cloned streams as there is no other activity that will cause an update to run.  Regular streams are created unsuspended so OnDataAvailable callbacks cause updates to be queued.

- Add IsSuspendedByDecoder() to media streams and use this in cache's Update() to avoid a cloned stream waiting on data from a sibling when the sibling has been suspended by the decoder.

All mochitests pass reliably for me with this patch.  I'm looking at posting another patch shortly that makes the cache block size variable based on the cache size--this is necessary for very small caches, e.g. the 100kB cache used in mochitests.  Without a variable block size it's possible to configure a cache that contains a single block, which probably results in poor performance due to heavy cache contention.

Comment 2

[Matthew Gregan [:kinetik]]

Comment on attachment 465459 [details] [diff] [review]
patch v0

(In reply to comment #1)
> All mochitests pass reliably for me with this patch.  I'm looking at posting
> another patch shortly that makes the cache block size variable based on the
> cache size--this is necessary for very small caches, e.g. the 100kB cache used
> in mochitests.  Without a variable block size it's possible to configure a
> cache that contains a single block, which probably results in poor performance
> due to heavy cache contention.

I discussed this with roc and we decided to avoid the extra complexity of variable block sizes for now.  If we ever increase the cache block size above 100kB (or whatever "small" cache the mochitests are trying to use) we'll need to come up with a solution.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 465459 [details] [diff] [review]
patch v0

nsMediaChannelStream::IsSuspendedByDecoder should be called IsSuspended, because any cause of suspension will make it return true. But that's what you want anyway.

Comment 4

[Matthew Gregan [:kinetik]]

Attachment: patch v1

Renamed IsSuspendedByDecoder() to IsSuspended().  Also changed the test in Update() from:

    !other->mCacheSuspended && !other->mClient->IsSuspendedByDecoder() &&

...to...

    !other->mClient->IsSuspended() &&

Since anytime mCacheSuspended is true mClient->IsSuspended() would return true.

Comment 5

[Matthew Gregan [:kinetik]]

Comment on attachment 465489 [details] [diff] [review]
patch v1

Requesting approval since this patch should improve performance and may fix some random orange in the media tests.  It's also needed to help performance in the WebM buffered implementation (bug 570904).

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

http://hg.mozilla.org/mozilla-central/rev/edbc3ebfd63c