Closed Bug 1365534 Opened 3 years ago Closed 3 years ago

Move FileBlockCache init/close file IOs off main thread

Categories

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

55 Branch
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: gerald, Assigned: gerald)

References

Details

Attachments

(3 files)

As far as I can tell, the only MediaCache IOs that can happen on the main thread are those in FileBlockCache::Init (non-e10s only), and in FileBlockCache::Close, to respectively open and close the temporary file used as cache repository.

These operations could be moved to the BlockFileCache thread, so that they cannot clog the main thread.
Comment on attachment 8868471 [details]
Bug 1365534 - In non-e10s, open temp file in FileBlockCache thread -

https://reviewboard.mozilla.org/r/140080/#review143718

::: dom/media/FileBlockCache.cpp:78
(Diff revision 2)
> +      PRFileDesc* fd = nullptr;
> +      nsresult rv = NS_OpenAnonymousTemporaryFile(&fd);
> -    if (NS_SUCCEEDED(rv)) {
> +      if (NS_SUCCEEDED(rv)) {
> -      mInitialized = true;
> +        self->SetCacheFile(fd);
> +      } else {
> +        self->Close();

Here you call Close() on a non-main thread, but Close() asserts that it's only called on the main thread.

I think it's safe to call Close() on a non-main thread, so you can just remove the assertion. Or, you could dispatch a runnable to the main thread to do lines 75-59 on the main thread.

Having said that, SetCacheFile() calls Close() if it's passed a null file descriptor, so assuming that fd==null when NS_OpenAnonymousTemporaryFile fails, you can just pass fd to SetCacheFile() irrespective of whether NS_OpenAnonymousTemporaryFile succeeded or not. It's a little less obvious that the error is handled if you do that however. Up to you.
Attachment #8868471 - Flags: review?(cpearce) → review+
Comment on attachment 8868472 [details]
Bug 1365534 - Close temp file in FileBlockCache thread -

https://reviewboard.mozilla.org/r/140082/#review143724
Attachment #8868472 - Flags: review?(cpearce) → review+
Comment on attachment 8868846 [details]
Bug 1365534 - Remove unneeded IsMainThread assertions -

https://reviewboard.mozilla.org/r/140430/#review143788

Historically, these were mostly documentary.
Attachment #8868846 - Flags: review?(cpearce) → review+
Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2ee0097a5aae
Remove unneeded IsMainThread assertions - r=cpearce
https://hg.mozilla.org/integration/autoland/rev/dcd4b4d5647b
In non-e10s, open temp file in FileBlockCache thread - r=cpearce
https://hg.mozilla.org/integration/autoland/rev/5f0510d514d6
Close temp file in FileBlockCache thread - r=cpearce
You need to log in before you can comment on or make changes to this bug.