Closed Bug 1347031 Opened 7 years ago Closed 7 years ago

Move the MediaCache off of opening its temporary file fd synchronously in the content process

Categories

(Core :: Audio/Video: Playback, enhancement, P1)

Product:
Core
Component:
Audio/Video: Playback
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Project Flags:
Performance Impact high
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: ehsan.akhgari, Assigned: cpearce)

References

Details

Attachments

(1 file)

Bug 1347031 - Move the MediaCache off of opening its temporary file fd synchronously in the content process.
59 bytes, text/x-review-board-request
jwwang
: review+
Details 
MediaCache::Init() opens mFileCache on the main thread synchronously right now, and we need to make this async in the content process at least (I'm hoping this isn't used in the parent process!)

This is used in two places: the first time gMediaCache is initialized, and when Flush() is called.

Right now, gMediaCache is initialized the first time a media element starts to receive data, or when last-pb-context-exited or cacheservice:empty-cache is dispatched.  The latter two are easy to deal with since they are initiated from the parent process.  Right now I'm planning to send an async request to initialize the cache at the time when gMediaCache gets initialized.  This runs the risk of not getting our cache file fd by the time we start receiving data, so I'd like to check to see whether that risk is acceptable here or not.  Alternative we could for example move this to happen shortly after we start the content process, but opening the file unconditionally seems wasteful.

What do you think, Chris?
Flags: needinfo?(cpearce)
(In reply to :Ehsan Akhgari from comment #0)
> MediaCache::Init() opens mFileCache on the main thread synchronously right
> now, and we need to make this async in the content process at least (I'm
> hoping this isn't used in the parent process!)

In theory, there's no reason why this couldn't be used from the parent process. So we need to handle that case I think.


> This is used in two places: the first time gMediaCache is initialized, and
> when Flush() is called.
> 
> Right now, gMediaCache is initialized the first time a media element starts
> to receive data, or when last-pb-context-exited or cacheservice:empty-cache
> is dispatched.  The latter two are easy to deal with since they are
> initiated from the parent process.  Right now I'm planning to send an async
> request to initialize the cache at the time when gMediaCache gets
> initialized.  This runs the risk of not getting our cache file fd by the
> time we start receiving data, so I'd like to check to see whether that risk
> is acceptable here or not.  Alternative we could for example move this to
> happen shortly after we start the content process, but opening the file
> unconditionally seems wasteful.
> 
> What do you think, Chris?

I think a simpler solution would be to push down the request for a temporary file handle into the FileBlockCache itself. It already buffers writes and defers then to another thread. So adding another async step before the FileBlockCache::Run() processes the change list that waits until the file descriptor is valid should be pretty easy.
Component: Audio/Video → Audio/Video: Playback
Flags: needinfo?(cpearce)
Ehsan: I can take this bug if you like. It's up my alley, which makes a nice change for a quantum-flow bug.
Priority: -- → P1
Yes, I would appreciate your help!  I'm landing bug 1346987 on inbound so you can use IPC code from that bug.  :-)
(Maybe we can do something like EncodedBufferCache::AppendBuffer() and reuse much more code)
Assignee: ehsan → cpearce
Whiteboard: [qf:p1]
Note: the patches here are based on top of bug 1346987, which was backed out.
Comment on attachment 8847874 [details]
Bug 1347031 - Move the MediaCache off of opening its temporary file fd synchronously in the content process.

https://reviewboard.mozilla.org/r/120790/#review122808

::: dom/media/FileBlockCache.cpp:141
(Diff revision 3)
>  
>    return NS_OK;
>  }
>  
> +RefPtr<GenericPromise>
> +FileBlockCache::EnsureInitialized()

It will be a lot simpler to do EnsureInitialized() in Init() once and for all.

1. Since Init() is called on the main thread, you don't need to dispatch a task to call ContentChild::AsyncOpenAnonymousTemporaryFile().
2. You don't need locks for synchronization for all code should run after Init() is done.

RefPtr<GenericPromise::Private> mInitPromise;

void EnsureInitialized()
{
  if (XRE_IsParentProcess()) {
    nsresult rv = NS_OpenAnonymousTemporaryFile(&mFD);
    mInitPromise = NS_SUCCEEDED(rv)
                 ? GenericPromise::CreateAndResolve()
                 : GenericPromise::CreateAndReject();
    return;
  }
  mInitPromise = new GenericPromise::Private();
  dom::ContentChild::GetSingleton()->AsyncOpenAnonymousTemporaryFile(
    [self](PRFileDesc* aFD) {
      self->mFD = aFD; // No synchronization since all access to mFD happens after mInitPromise is resolved.
      self->mInitPromise->Resolve(); // Resolve() is thread-safe.
      // Handle aFD is null.
    }
  );
}

void EnsureWriteScheduled()
{
  mInitPromise->Then(
    mAbstractThread, __func__,
    []() { self->Run(); } // self->Run() should only happen mInitPromise is resolved.
    []() {} // Do nothing since this is already handled by EnsureInitialized().
  );
}
Comment on attachment 8847874 [details]
Bug 1347031 - Move the MediaCache off of opening its temporary file fd synchronously in the content process.

https://reviewboard.mozilla.org/r/120790/#review123244

::: dom/media/FileBlockCache.cpp:62
(Diff revision 4)
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> -    MonitorAutoLock mon(mDataMonitor);
> +  MonitorAutoLock mon(mDataMonitor);
> -    nsresult res = NS_NewNamedThread("FileBlockCache",
> +  nsresult rv;
> +
> +  if (XRE_IsParentProcess()) {

Move this below NS_NewNamedThread(). So SetCacheFile() always sees mIsOpen is true.

::: dom/media/FileBlockCache.cpp:67
(Diff revision 4)
> +  if (XRE_IsParentProcess()) {
> +    rv = NS_OpenAnonymousTemporaryFile(&mFD);
> +    if (NS_FAILED(rv)) {
> +      return rv;
> +    }
> +    mInitPromise->Resolve(true, __func__);

It is a null-deref here. Need to create an object first.
Attachment #8847874 - Flags: review?(jwwang) → review+
Comment on attachment 8847874 [details]
Bug 1347031 - Move the MediaCache off of opening its temporary file fd synchronously in the content process.

https://reviewboard.mozilla.org/r/120790/#review123276

::: dom/media/FileBlockCache.cpp:62
(Diff revision 4)
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> -    MonitorAutoLock mon(mDataMonitor);
> +  MonitorAutoLock mon(mDataMonitor);
> -    nsresult res = NS_NewNamedThread("FileBlockCache",
> +  nsresult rv;
> +
> +  if (XRE_IsParentProcess()) {

I deliberately moved it up so that if NS_OpenAnonymousTemporaryFile fails we don't need to worry about shutting down the thread.

AsyncOpenAnonymousTemporaryFile is async, so we can't get a callback to SetCacheFile befofe mIsOpen is initialized.

::: dom/media/FileBlockCache.cpp:67
(Diff revision 4)
> +  if (XRE_IsParentProcess()) {
> +    rv = NS_OpenAnonymousTemporaryFile(&mFD);
> +    if (NS_FAILED(rv)) {
> +      return rv;
> +    }
> +    mInitPromise->Resolve(true, __func__);

Very true, thanks!
Comment on attachment 8847874 [details]
Bug 1347031 - Move the MediaCache off of opening its temporary file fd synchronously in the content process.

https://reviewboard.mozilla.org/r/120790/#review123276

> I deliberately moved it up so that if NS_OpenAnonymousTemporaryFile fails we don't need to worry about shutting down the thread.
> 
> AsyncOpenAnonymousTemporaryFile is async, so we can't get a callback to SetCacheFile befofe mIsOpen is initialized.

You still need to worry about the thread shutdown when AsyncOpenAnonymousTemporaryFile fails. The error handling is simpler if open-file is done after creating a thread since you always need to shut down the thread no matter it is in parent or child process. It is less performant though. However, given the chance is low failing to open a file handle, I prefer simplicity over performance.
Comment on attachment 8847874 [details]
Bug 1347031 - Move the MediaCache off of opening its temporary file fd synchronously in the content process.

https://reviewboard.mozilla.org/r/120790/#review123276

> You still need to worry about the thread shutdown when AsyncOpenAnonymousTemporaryFile fails. The error handling is simpler if open-file is done after creating a thread since you always need to shut down the thread no matter it is in parent or child process. It is less performant though. However, given the chance is low failing to open a file handle, I prefer simplicity over performance.

Ah yes, I had neglected to check whether the AsyncOpenAnonymousTemporaryFile call itself had failed to send to the parent process. I'll check that and re-arrange as you suggest. Thanks!
Pushed by cpearce@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8b0d4d072cf0
Move the MediaCache off of opening its temporary file fd synchronously in the content process. r=jwwang
https://hg.mozilla.org/mozilla-central/rev/8b0d4d072cf0
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Performance Impact: --- → P1
Whiteboard: [qf:p1]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: