Last Comment Bug 736342 - Don't write media cache data on the main thread
: Don't write media cache data on the main thread
Status: RESOLVED FIXED
: main-thread-io
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: 11 Branch
: All All
: -- normal (vote)
: mozilla14
Assigned To: Chris Pearce (:cpearce)
:
Mentors:
Depends on: 741052
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-15 18:59 PDT by Chris Pearce (:cpearce)
Modified: 2012-03-31 00:43 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch: Defer media cache writes to non main thread. (26.64 KB, patch)
2012-03-24 15:45 PDT, Chris Pearce (:cpearce)
no flags Details | Diff | Splinter Review
Patch 1 v1: AsyncBlockWriter (18.17 KB, patch)
2012-03-25 21:27 PDT, Chris Pearce (:cpearce)
no flags Details | Diff | Splinter Review
Patch 2 v1: Use AsyncBlockWriter in media cache (10.71 KB, patch)
2012-03-25 21:28 PDT, Chris Pearce (:cpearce)
no flags Details | Diff | Splinter Review
Patch 1 v2: AsyncBlockWriter (22.81 KB, patch)
2012-03-27 20:49 PDT, Chris Pearce (:cpearce)
roc: review+
Details | Diff | Splinter Review
Patch 2 v2: Use AsyncBlockWriter in media cache (10.89 KB, patch)
2012-03-27 20:49 PDT, Chris Pearce (:cpearce)
roc: review+
Details | Diff | Splinter Review

Description Chris Pearce (:cpearce) 2012-03-15 18:59:06 PDT
The media cache writes data to disk in its OnDataAvailable listener. This runs on the main thread, which can block waiting for IO to complete, particularly when the system is performing lots of IO.

For example, when I'm unzipping a zip of my objdir while playing a YouTube HTML5 video, I ran into a pause with the following main thread stack:

ntdll.dll!_NtWriteFile@36()  + 0x15 bytes	
ntdll.dll!_NtWriteFile@36()  + 0x15 bytes	
kernel32.dll!_WriteFileImplementation@20()  + 0x4a bytes	
nspr4.dll!FileWrite(PRFileDesc * fd, const void * buf, int amount)  Line 109 + 0x8 bytes	C
nspr4.dll!PR_Write(PRFileDesc * fd, const void * buf, int amount)  Line 146 + 0x13 bytes	C
xul.dll!nsMediaCache::Write(const void * aData, int aLength)  Line 402 + 0x11 bytes	C++
xul.dll!nsMediaCache::WriteCacheFile(__int64 aOffset, const void * aData, int aLength)  Line 894 + 0x9 bytes	C++
xul.dll!nsMediaCache::AllocateAndWriteBlock(nsMediaCacheStream * aStream, const void * aData, nsMediaCacheStream::ReadMode aMode)  Line 1721 + 0xa bytes	C++
xul.dll!nsMediaCacheStream::NotifyDataReceived(__int64 aSize, const char * aData, nsIPrincipal * aPrincipal)  Line 1991	C++
xul.dll!mozilla::ChannelMediaResource::CopySegmentToCache(nsIInputStream * aInStream, void * aClosure, const char * aFromSegment, unsigned int aToOffset, unsigned int aCount, unsigned int * aWriteCount)  Line 381	C++
xul.dll!nsPipeInputStream::ReadSegments(unsigned int (nsIInputStream *, void *, const char *, unsigned int, unsigned int, unsigned int *)* writer, void * closure, unsigned int count, unsigned int * readCount)  Line 799 + 0x13 bytes	C++
xul.dll!mozilla::ChannelMediaResource::OnDataAvailable(nsIRequest * aRequest, nsIInputStream * aStream, unsigned int aCount)  Line 409	C++
xul.dll!mozilla::ChannelMediaResource::Listener::OnDataAvailable(nsIRequest * aRequest, nsISupports * aContext, nsIInputStream * aStream, unsigned int aOffset, unsigned int aCount)  Line 128	C++
xul.dll!nsHTMLMediaElement::MediaLoadListener::OnDataAvailable(nsIRequest * aRequest, nsISupports * aContext, nsIInputStream * aStream, unsigned int aOffset, unsigned int aCount)  Line 388	C++
xul.dll!nsHttpChannel::OnDataAvailable(nsIRequest * request, nsISupports * ctxt, nsIInputStream * input, unsigned int offset, unsigned int count)  Line 4478 + 0x36 bytes	C++
xul.dll!nsInputStreamPump::OnStateTransfer()  Line 514 + 0x18 bytes	C++
xul.dll!nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream * stream)  Line 403	C++
xul.dll!nsOutputStreamReadyEvent::Run()  Line 115	C++
xul.dll!nsThread::ProcessNextEvent(bool mayWait, bool * result)  Line 657 + 0x9 bytes	C++
xul.dll!NS_ProcessNextEvent_P(nsIThread * thread, bool mayWait)  Line 245 + 0xd bytes	C++
xul.dll!mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate * aDelegate)  Line 134 + 0xa bytes	C++
xul.dll!MessageLoop::RunHandler()  Line 202	C++
xul.dll!MessageLoop::Run()  Line 176	C++
xul.dll!nsBaseAppShell::Run()  Line 191	C++
xul.dll!nsAppShell::Run()  Line 252 + 0x6 bytes	C++
xul.dll!nsAppStartup::Run()  Line 296	C++
xul.dll!XRE_main(int argc, char * * argv, const nsXREAppData * aAppData)  Line 3564 + 0xc bytes	C++
firefox.exe!do_main(int argc, char * * argv)  Line 190 + 0x12 bytes	C++
firefox.exe!NS_internal_main(int argc, char * * argv)  Line 277 + 0xe bytes	C++
firefox.exe!wmain(int argc, wchar_t * * argv)  Line 109	C++
firefox.exe!__tmainCRTStartup()  Line 552 + 0x17 bytes	C
kernel32.dll!@BaseThreadInitThunk@12()  + 0x12 bytes	
ntdll.dll!___RtlUserThreadStart@8()  + 0x27 bytes	
ntdll.dll!__RtlUserThreadStart@8()  + 0x1b bytes	

So I propose we have the media cache create its own thread for writing data to cache, and in ChannelMediaResource::OnDataAvailable we copy incoming data into a memory buffer and dispatch an event to write it on the media cache's IO thread.
Comment 1 Josh Matthews [:jdm] 2012-03-16 11:01:52 PDT
If a media person wants to mark themselves as a mentor instead, I would have no problem with that. The code changes here don't look particularly complex:

* add an nsIThread member to ChannelMediaResource (we can use LazyIdleThread for the actual thread object that is created for bonus points (see http://mxr.mozilla.org/mozilla-central/source/xpcom/threads/LazyIdleThread.h))

* create a class that derives from nsRunnable that basically has the contents of ChannelMediaResource::CopySegmentToCache inside its Run method (except for |*aWriteCount = aCount|; I think that needs to stay where it is). This runnable needs to create a char* buffer of aCount size and copy the data from aFromSegment so that it's available on the other thread

* CopySegmentToCache creates an instance of the new runnable and dispatches it to the nsIThread that ChannelMediaResource has

* we may need to create another runnable that calls  |closure->mResource->mDecoder->NotifyDataArrived(aFromSegment, aCount, closure->mResource->mOffset);| on the main thread - this is where a media person could help; we may be able to leave it in CopySegmentToCache and avoid this problem. http://mxr.mozilla.org/mozilla-central/source/content/media/webm/nsWebMBufferedParser.cpp#260 specifically asserts that it is on the main thread, at least.

From a fairly cursory look, I think these steps would fix the problem.
Comment 2 Chris Pearce (:cpearce) 2012-03-18 21:59:16 PDT
I don't think it's that simple. We also do main thread writes in nsMediaCache::Update() and nsMediaCacheStream::NotifyDataEnded(). Not sure what the best solution is, I'm still thinking about it...
Comment 3 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-03-19 02:18:39 PDT
The problem is that writing asynchronously breaks the invariant that the data is available for reading immediately.

Maybe the best option would be to introduce a new abstraction for "large random-access file-backed buffer" that offers synchronous, thread-safe reading and writing APIs, but where the writing API doesn't actually do disk I/O, just accumulates data in memory and writes it out from another thread. Of course the reading API needs to be able to find and return data from the pending-writes buffer.

Under heavy load this might just replace "waiting for file write" with "waiting for pagefile allocation", but I don't know if there's a way to do better.
Comment 4 Chris Pearce (:cpearce) 2012-03-24 15:45:59 PDT
Created attachment 609041 [details] [diff] [review]
Patch: Defer media cache writes to non main thread.

Defers media cache writes and block moves to a non-main thread by wrapping I/O in an AsyncBlockWriter class.

Greenish on Try:
https://tbpl.mozilla.org/?tree=Try&rev=5b7aa02d72ce
Comment 5 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-03-25 16:04:37 PDT
Comment on attachment 609041 [details] [diff] [review]
Patch: Defer media cache writes to non main thread.

Review of attachment 609041 [details] [diff] [review]:
-----------------------------------------------------------------

Put AsyncBlockWriter in its own files and its own patch please
Comment 6 Chris Pearce (:cpearce) 2012-03-25 21:27:37 PDT
Created attachment 609202 [details] [diff] [review]
Patch 1 v1: AsyncBlockWriter

Move AsyncBlockWriter to its own file.
Comment 7 Chris Pearce (:cpearce) 2012-03-25 21:28:48 PDT
Created attachment 609203 [details] [diff] [review]
Patch 2 v1: Use AsyncBlockWriter in media cache

Green on Try: https://tbpl.mozilla.org/?tree=Try&rev=e4153000e9df
Comment 8 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-03-26 03:54:06 PDT
Comment on attachment 609202 [details] [diff] [review]
Patch 1 v1: AsyncBlockWriter

Review of attachment 609202 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/AsyncBlockWriter.cpp
@@ +175,5 @@
> +      // from disk.
> +      nsresult res = ReadFromDisk(blockIndex * BLOCK_SIZE + start,
> +                                  dst,
> +                                  amount,
> +                                  bytesRead);

This holds the monitor while reading from disk. So main-thread activity that needs the monitor (e.g., a call to MoveBlock or WriteBlock) will block waiting for this read. That seems bad.

::: content/media/AsyncBlockWriter.h
@@ +68,5 @@
> +  // Closes writer, shuts down thread.
> +  void Close();
> +
> +  // Can be called on any thread. This defers to a non-main thread.
> +  nsresult WriteBlock(PRUint32 aBlockIndex, const void* aData);

Prefer PRUint8 for the data type (and in BlockChange)

@@ +87,5 @@
> +  nsresult MoveBlock(PRInt32 aSourceBlockIndex, PRInt32 aDestBlockIndex);
> +
> +  // Represents a change yet to be made to a block on disk. The change is
> +  // either a write (and the data to be written is stored in this struct)
> +  // or its a move (and the index of the source block is stored instead).

delete "its"

@@ +139,5 @@
> +  // cached in memory waiting to be written, or this block is the target of a
> +  // block move.
> +  nsTArray< nsAutoPtr<BlockChange> > mBlockChanges;
> +
> +  // Thread upon which block writes and block moves are preformed. This is

performed
Comment 9 Chris Pearce (:cpearce) 2012-03-27 20:49:00 PDT
Created attachment 609996 [details] [diff] [review]
Patch 1 v2: AsyncBlockWriter

Add split locking into two locks, one for the file IO related fields, the other for the data fields.
Comment 10 Chris Pearce (:cpearce) 2012-03-27 20:49:42 PDT
Created attachment 609997 [details] [diff] [review]
Patch 2 v2: Use AsyncBlockWriter in media cache
Comment 11 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-03-27 21:36:06 PDT
Comment on attachment 609996 [details] [diff] [review]
Patch 1 v2: AsyncBlockWriter

Review of attachment 609996 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/AsyncBlockWriter.cpp
@@ +151,5 @@
> +{
> +  mFileMonitor.AssertCurrentThreadOwns();
> +
> +  nsresult res = Seek(aBlockIndex * BLOCK_SIZE);
> +  if (NS_FAILED(res)) return res;

rv, not res.

@@ +164,5 @@
> +  return NS_OK;
> +}
> +
> +nsresult AsyncBlockWriter::MoveBlockOnDisk(PRInt32 aSourceBlockIndex,
> +                                           PRInt32 aDestBlockIndex)

Better to say "File" instead of "Disk" since it might not be a disk.

::: content/media/AsyncBlockWriter.h
@@ +109,5 @@
> +
> +  class Int32Queue : private nsDeque {
> +  public:
> +
> +    PRInt32 PopFront() {

Unnecessary blank line

@@ +134,5 @@
> +    }
> +
> +  private:
> +
> +    PRInt32 ObjectAt(PRInt32 aIndex) {

Unnecessary blank line

@@ +144,5 @@
> +  };
> +
> +private:
> +
> +  // Ensures we either are running the event to preform IO, or an event

Unnecessary blank line

@@ +169,5 @@
> +  nsresult WriteBlockToDisk(PRInt32 aBlockIndex, const PRUint8* aBlockData);
> +
> +  // Monitor which controls access to mFD and mFDCurrentPos. Don't hold
> +  // mDataMonitor while holding mFileMonitor!
> +  mozilla::Monitor mFileMonitor;

Group methods and fields that need mFileMonitor to be held together and put them under mFileMonitor.

@@ +180,5 @@
> +  PRInt64 mFDCurrentPos;
> +
> +  // Monitor which controls access to alldata in this class, except mFD
> +  // and mFDCurrentPos. Don't hold mDataMonitor while holding mFileMonitor!
> +  mozilla::Monitor mDataMonitor;

Group methods and fields that need mDataMonitor to be held together and put them under mDataMonitor .
Comment 12 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-03-27 21:46:44 PDT
Comment on attachment 609997 [details] [diff] [review]
Patch 2 v2: Use AsyncBlockWriter in media cache

Review of attachment 609997 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/nsMediaCache.cpp
@@ +50,5 @@
>  #include "nsMathUtils.h"
>  #include "prlog.h"
>  #include "nsIPrivateBrowsingService.h"
>  #include "mozilla/Preferences.h"
> +#include "AsyncBlockWriter.h"

Actually I think AsyncBlockWriter isn't a great name.

How about BlockFileCache or FileBlockCache?

@@ +1133,5 @@
> +              blockIndex, destinationBlockIndex));
> +          // Swapping the block metadata here lets us maintain the
> +          // correct positions in the linked lists
> +          SwapBlocks(blockIndex, destinationBlockIndex);
> +          //Free the overflowing block even if the copy failed.

Space after //
Comment 13 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-03-27 21:50:21 PDT
Comment on attachment 609996 [details] [diff] [review]
Patch 1 v2: AsyncBlockWriter

Review of attachment 609996 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/AsyncBlockWriter.h
@@ +16,5 @@
> +namespace mozilla {
> +
> +// Manages file I/O for the media cache. Data comes in over the network
> +// via callbacks on the main thread, however we don't want to write the
> +// incoming data to the media cache on the main thread, as this could block

I think it would be good to add to this comment so it describes what a AsyncBlockWriter actually is. It's an abstraction for a temporary file accessible as an array of blocks, which supports a block move operation, and allows synchronous reading and writing from any thread, with writes being buffered so as not to block.

Note You need to log in before you can comment on or make changes to this bug.