1274522 - Use AsyncShutdown API in MediaShutdownManager

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P2)

Description

[JW Wang [:jwwang] [:jw_wang]]

This will allow us to remove creepy spinning event loop.

Comment 1

[JW Wang [:jwwang] [:jw_wang]]

Attachment: MozReview Request: Bug 1274522 - Use AsyncShutdown API in MediaShutdownManager.

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

Comment 2

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

Comment on attachment 8755250 [details]
MozReview Request: Bug 1274522 - Use AsyncShutdown API in MediaShutdownManager.

https://reviewboard.mozilla.org/r/54466/#review51202

r+ for the code.
A couple of log messages to fix, and maybe the big comment text in the header file as well:

::: dom/media/MediaShutdownManager.h:40
(Diff revision 1)
>  // The MediaShutdownManager ensures that the MediaDecoder stack is shutdown
>  // before returning from its xpcom-shutdown observer by keeping track of all

There is no explicit observer here anymore; does this whole comment section need some rewriting to explain how it now works with the nsIAsyncShutdownBlocker?

::: dom/media/MediaShutdownManager.cpp:82
(Diff revision 1)
>      } else {
> -      nsContentUtils::UnregisterShutdownObserver(this);
> +      GetShutdownBarrier()->RemoveBlocker(this);
>        // Clear our singleton reference. This will probably delete
>        // this instance, so don't deref |this| clearing sInstance.
>        sInstance = nullptr;
> +      DECODER_LOG(LogLevel::Debug, ("MediaShutdownManager::Shutdown() end."));

There is no more 'MediaShutdownManager::Shutdown()', so this message looks wrong.

::: dom/media/MediaShutdownManager.cpp:129
(Diff revision 1)
> -MediaShutdownManager::Shutdown()
> +MediaShutdownManager::BlockShutdown(nsIAsyncShutdownClient*)
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>    MOZ_ASSERT(sInstance);
>  
>    DECODER_LOG(LogLevel::Debug, ("MediaShutdownManager::Shutdown() start..."));

No 'Shutdown()' anymore.

Comment 3

[Jean-Yves Avenard [:jya]]

Comment on attachment 8755250 [details]
MozReview Request: Bug 1274522 - Use AsyncShutdown API in MediaShutdownManager.

https://reviewboard.mozilla.org/r/54466/#review51346

LGTM, but I don't believe to be the right person to do this review, I have no knowledge of the async shutdown whatsoever.

::: dom/media/MediaShutdownManager.cpp:63
(Diff revision 1)
> +    // We are probably in a content process.
> +    rv = svc->GetContentChildShutdown(getter_AddRefs(barrier));
> +  }
> +  MOZ_RELEASE_ASSERT(NS_SUCCEEDED(rv));
> +  MOZ_RELEASE_ASSERT(barrier);
> +  return barrier.forget();

your prototyping indicates that you are returning an nsCOMPtr.

But you return barrier.forget()

wouldn't that return an already_AddRefed which would have to be converted to nsCOMPtr once again.

Shouldn't you directly return barrier ?

Comment 4

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Jean-Yves Avenard [:jya] from comment #3)
> your prototyping indicates that you are returning an nsCOMPtr.
> 
> But you return barrier.forget()
> 
> wouldn't that return an already_AddRefed which would have to be converted to
> nsCOMPtr once again.
> 
> Shouldn't you directly return barrier ?

IIUC, without return-value-optimization the return value will be copy-constructed from |barrier| and incur additional AddRef/Release if we just return |barrier| instead of |barrier.forget()|.

Comment 5

[Jean-Yves Avenard [:jya]]

(In reply to JW Wang [:jwwang] from comment #4)

> IIUC, without return-value-optimization the return value will be
> copy-constructed from |barrier| and incur additional AddRef/Release if we
> just return |barrier| instead of |barrier.forget()|.

Hmm.. it seems that nsCOMPtr doesn't have a move constructor anyway.. how silly, it has a move constructor from already_AddRefed.

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

Attachment: MozReview Request: Bug 1274522. Part 1 - Use AsyncShutdown API in MediaShutdownManager. r=gerald.

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

Comment 7

[JW Wang [:jwwang] [:jw_wang]]

Attachment: MozReview Request: Bug 1274522. Part 2 - fix comments. r=gerald.

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

Comment 8

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8755689 [details]
MozReview Request: Bug 1274522. Part 1 - Use AsyncShutdown API in MediaShutdownManager. r=gerald.

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

Comment 9

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8755690 [details]
MozReview Request: Bug 1274522. Part 2 - fix comments. r=gerald.

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

Comment 10

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

Comment on attachment 8755690 [details]
MozReview Request: Bug 1274522. Part 2 - fix comments. r=gerald.

https://reviewboard.mozilla.org/r/54748/#review51394

Thank you.

Comment 11

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

Comment on attachment 8755689 [details]
MozReview Request: Bug 1274522. Part 1 - Use AsyncShutdown API in MediaShutdownManager. r=gerald.

https://reviewboard.mozilla.org/r/54746/#review51396

(Just refreshing r+)

Comment 12

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

(In reply to JW Wang [:jwwang] from comment #4)
> (In reply to Jean-Yves Avenard [:jya] from comment #3)
> > your prototyping indicates that you are returning an nsCOMPtr.
> > 
> > But you return barrier.forget()
> > 
> > wouldn't that return an already_AddRefed which would have to be converted to
> > nsCOMPtr once again.
> > 
> > Shouldn't you directly return barrier ?
> 
> IIUC, without return-value-optimization the return value will be
> copy-constructed from |barrier| and incur additional AddRef/Release if we
> just return |barrier| instead of |barrier.forget()|.

But wouldn't there be RVO in your case? |barrier| is of the exact returned type, and constructed on the stack, so I think it's a good candidate for RVO, if you just did |return barrier;|. (Even if there's no move-constructor)
You might have to examine the produced code, or trace it, to be sure.

Now, in that case it probably doesn't matter if a few cycles are lost, as it's not critical code.
But it might be good to investigate anyway, for future code we all will write!

Comment 13

[JW Wang [:jwwang] [:jw_wang]]

RVO was able to kick in in GCC even with "-O0".

However, it can't with the code blow:
static Foo(bool b) {
  Foo foo1;
  Foo foo2;
  if (b) return foo1;
  else return foo2;
}

My concern is the inconsistency in returning |barrier| when expecting RVO but |barrier.forget()| in cases like above.

Comment 14

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

(In reply to JW Wang [:jwwang] from comment #13)
> RVO was able to kick in in GCC even with "-O0".

Nice! I guess it's really part of the language, at the lowest level of optimization.

> However, it can't with the code blow:
> static Foo(bool b) {
>   Foo foo1; Foo foo2;
>   return b ? foo1 : foo2;
> }

Makes sense, the compiler needs to know which *one* local variable can be lifted into the caller's stack to hold the eventual return value.

> My concern is the inconsistency in returning |barrier| when expecting RVO
> but |barrier.forget()| in cases like above.

I like consistency too.
In which case I could argue that your function should really return an already_AddRefed<T>, to be consistent with the rest of our code. :-) (But you'd have to then explicitly construct nsCOMPtr's in the caller, that would turn ugly very quickly!)

We can be consistent by having 2 cases, e.g.: Always try RVO where possible, fall back to .forget() otherwise.
But this bug might not be the best place to argue style; maybe we should discuss this in mozilla.dev.platform...

Comment 15

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Gerald Squelart [:gerald] (may be slow to respond) from comment #14)
> In which case I could argue that your function should really return an
> already_AddRefed<T>, to be consistent with the rest of our code. :-)
I think that is the legacy when we don't have rvalue reference. I would always prefer to return a RefPtr<T> instead of an already_AddRefed<T> because it is more expressive and won't incur ref-counting overhead either.

> We can be consistent by having 2 cases, e.g.: Always try RVO where possible,
> fall back to .forget() otherwise.
> But this bug might not be the best place to argue style; maybe we should
> discuss this in mozilla.dev.platform...
Sure. I will start a new thread in the dev forum to draw more eyes and opinions.

Comment 16

[JW Wang [:jwwang] [:jw_wang]]

Thanks for the review.

I started a new thread:
https://groups.google.com/forum/#!topic/mozilla.dev.platform/MqZkC3IoDrk

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f05ea4c7e78b
https://hg.mozilla.org/integration/mozilla-inbound/rev/9036d334851e

Comment 18

[Joel Maher ( :jmaher ) (UTC -8)]

thanks for the patches here, we see a 5% tp5o_scroll win8 improvement!!
https://treeherder.mozilla.org/perf.html#/alerts?id=1306

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f05ea4c7e78b
https://hg.mozilla.org/mozilla-central/rev/9036d334851e