Increase media cache block size from 4kB to 32kB

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: kinetik, Assigned: kinetik)

Tracking

Trunk
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
Posted patch patch v0 (obsolete) — Splinter Review
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.
Assignee: nobody → kinetik
Status: NEW → ASSIGNED
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.
Attachment #465459 - Flags: review?(roc)
Blocks: 570904
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.
Attachment #465459 - Flags: review?(roc) → review+
Posted patch patch v1Splinter Review
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.
Attachment #465459 - Attachment is obsolete: true
Attachment #465489 - Flags: review+
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).
Attachment #465489 - Flags: approval2.0?
Keywords: checkin-needed
Whiteboard: [needs landing]
http://hg.mozilla.org/mozilla-central/rev/edbc3ebfd63c
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing]
You need to log in before you can comment on or make changes to this bug.