1365534 - Move FileBlockCache init/close file IOs off main thread

Status: RESOLVED FIXED

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

Description

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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 1

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1365534 - In non-e10s, open temp file in FileBlockCache thread -

In a parent process, the temporary file was created and opened by calling
NS_OpenAnonymousTemporaryFile() from Init, which was called from the main
thread.
In addition to being called at least once when the first media file is fetched,
it may also be called if caches need to be emptied.

So it should help to move this operation on the FileBlockCache thread, if only
to remove potential background-hang reports from non-e10s configurations.

Review commit: https://reviewboard.mozilla.org/r/140080/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/140080/

Comment 2

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1365534 - Close temp file in FileBlockCache thread -

Closing the temporary file was done in the FileBlockCache destructor, which is
called on the main thread.

Instead, we are now dispatching a new final task from Close, which will close
the temporary file and then shutdown the thread.
The thread ownership is transferred to that final task, so that no more tasks
may be unexpectly queued to that thread.

Review commit: https://reviewboard.mozilla.org/r/140082/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/140082/

Comment 3

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8868471 [details]
Bug 1365534 - In non-e10s, open temp file in FileBlockCache thread -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140080/diff/1-2/

Comment 4

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8868472 [details]
Bug 1365534 - Close temp file in FileBlockCache thread -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140082/diff/1-2/

Comment 5

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

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.

Comment 6

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

Comment on attachment 8868472 [details]
Bug 1365534 - Close temp file in FileBlockCache thread -

https://reviewboard.mozilla.org/r/140082/#review143724

Comment 7

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1365534 - Remove unneeded IsMainThread assertions -

SetCacheFile, Init, Close, and MoveBlock all had NS_IsMainThread() assertions.
However they all use Monitor's to access member data, so they should be thread-
safe.
(Upcoming patches will actually start using some of these functions from non-
main-threads.)

Review commit: https://reviewboard.mozilla.org/r/140430/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/140430/

Comment 8

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8868471 [details]
Bug 1365534 - In non-e10s, open temp file in FileBlockCache thread -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140080/diff/2-3/

Comment 9

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8868472 [details]
Bug 1365534 - Close temp file in FileBlockCache thread -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140082/diff/2-3/

Comment 10

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

Comment on attachment 8868846 [details]
Bug 1365534 - Remove unneeded IsMainThread assertions -

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

Historically, these were mostly documentary.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
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

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2ee0097a5aae
https://hg.mozilla.org/mozilla-central/rev/dcd4b4d5647b
https://hg.mozilla.org/mozilla-central/rev/5f0510d514d6