1411523 - Remove 'this' from lambda captures [self, this] under dom/media

Status: RESOLVED FIXED

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

Description

[James Cheng[:JamesCheng]]

This redundant to capture 'this' since we've already captured self for extending the live time.

We can use self instead of this.

It can save a pointer copy and make the code more readable that we can clearly understand the method we invoked is a self's method but a global function.

With inlined RefPtr::operator->, I think it is reasonable to make this change.

Comment 1

[James Cheng[:JamesCheng]]

Attachment: Bug 1411523 - Remove 'this' from lambda captures [self, this] under dom/media

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

Comment 2

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

I think |this| is is captured to save typing. E.g. self->DoSomething() v.s DoSomething().

Comment 3

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

As JW said, 'this' was captured to enable easier access to members.
I personally prefer removing it, using 'self' explicitly when accessing members; But I will transfer the final decision to JW.

(I'm moving away from Media, so it would feel wrong for me to decide the style inside dom/media ;-) )

Comment 4

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

Comment on attachment 8921810 [details]
Bug 1411523 - Remove 'this' from lambda captures [self, this] under dom/media

https://reviewboard.mozilla.org/r/192838/#review198086

::: dom/media/platforms/agnostic/AOMDecoder.cpp:84
(Diff revision 1)
>  RefPtr<ShutdownPromise>
>  AOMDecoder::Shutdown()
>  {
>    RefPtr<AOMDecoder> self = this;
> -  return InvokeAsync(mTaskQueue, __func__, [self, this]() {
> -    auto res = aom_codec_destroy(&mCodec);
> +  return InvokeAsync(mTaskQueue, __func__, [self]() {
> +    auto res = aom_codec_destroy(&(self->mCodec));

Just |&self->mCodec| will do.

::: dom/media/platforms/agnostic/VPXDecoder.cpp:91
(Diff revision 1)
>  VPXDecoder::Shutdown()
>  {
>    RefPtr<VPXDecoder> self = this;
> -  return InvokeAsync(mTaskQueue, __func__, [self, this]() {
> -    vpx_codec_destroy(&mVPX);
> -    vpx_codec_destroy(&mVPXAlpha);
> +  return InvokeAsync(mTaskQueue, __func__, [self]() {
> +    vpx_codec_destroy(&(self->mVPX));
> +    vpx_codec_destroy(&(self->mVPXAlpha));

Ditto.

::: dom/media/platforms/agnostic/VorbisDecoder.cpp:283
(Diff revision 1)
>    RefPtr<VorbisDataDecoder> self = this;
> -  return InvokeAsync(mTaskQueue, __func__, [self, this]() {
> +  return InvokeAsync(mTaskQueue, __func__, [self]() {
>      // Ignore failed results from vorbis_synthesis_restart. They
>      // aren't fatal and it fails when ResetDecode is called at a
>      // time when no vorbis data has been read.
> -    vorbis_synthesis_restart(&mVorbisDsp);
> +    vorbis_synthesis_restart(&(self->mVorbisDsp));

Ditto.

::: dom/media/platforms/android/RemoteDataDecoder.cpp:495
(Diff revision 1)
>      }
> -    RefPtr<DecodePromise> p = mDrainPromise.Ensure(__func__);
> -    if (mDrainStatus == DrainStatus::DRAINED) {
> +    RefPtr<DecodePromise> p = self->mDrainPromise.Ensure(__func__);
> +    if (self->mDrainStatus == DrainStatus::DRAINED) {
>        // There's no operation to perform other than returning any already
>        // decoded data.
>        ReturnDecodedData();

Need no "self->"?

::: dom/media/platforms/omx/OmxDataDecoder.cpp:138
(Diff revision 1)
>    RefPtr<OmxDataDecoder> self = this;
>    mOmxLayer->SendCommand(OMX_CommandFlush, OMX_ALL, nullptr)
>      ->Then(mOmxTaskQueue, __func__,
> -        [self, this] () {
> -          mDrainPromise.ResolveIfExists(mDecodedData, __func__);
> -            mDecodedData.Clear();
> +        [self] () {
> +          self->mDrainPromise.ResolveIfExists(self->mDecodedData, __func__);
> +            self->mDecodedData.Clear();

indentation.

Comment 5

[Jean-Yves Avenard [:jya]]

(In reply to Gerald Squelart [:gerald] from comment #3)
> As JW said, 'this' was captured to enable easier access to members.
> I personally prefer removing it, using 'self' explicitly when accessing
> members; But I will transfer the final decision to JW.
> 
> (I'm moving away from Media, so it would feel wrong for me to decide the
> style inside dom/media ;-) )

I don't :)

plus the description of the bug seems to assume that the saving of a single pointer copy will be better that all the new call to operator->() which will dereference self.

even if inlined, it is still slower than using this

Comment 6

[Jean-Yves Avenard [:jya]]

Comment on attachment 8921810 [details]
Bug 1411523 - Remove 'this' from lambda captures [self, this] under dom/media

https://reviewboard.mozilla.org/r/192838/#review198114

::: commit-message-295ed:2
(Diff revision 1)
> +Bug 1411523 - Remove 'this' from lambda captures [self, this] under dom/media. r?gerald
> +

I don't see how this change makes the code clearer in any way shapes or forms

Comment 7

[Jean-Yves Avenard [:jya]]

Comment on attachment 8921810 [details]
Bug 1411523 - Remove 'this' from lambda captures [self, this] under dom/media

https://reviewboard.mozilla.org/r/192838/#review198086

> Need no "self->"?

I doubt that compile...

Comment 8

[Jean-Yves Avenard [:jya]]

the code is certainly not inlined:

http://searchfox.org/mozilla-central/source/mfbt/RefPtr.h#315

which calls get()
which isn't inline either.

Stepping in into self-> operator, clearly shows of the additional code required.

If the basis for this change was because it was as efficient to dereference this vs dereference RefPtr self, then this should be r-

Comment 9

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

I prefer explicit over implicit. It is more clear to use self-> than calling a member without this->. However, it is quite annoying to see self-> appear many times in a single lambda.

Btw, I don't care much about the performance of dereference since those lambdas are not hot code.

Comment 10

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

(In reply to Jean-Yves Avenard [:jya] from comment #8)
> If the basis for this change was because it was as efficient to dereference
> this vs dereference RefPtr self, then this should be r-

Just go to the review board and r- to make a balance. :)

Comment 11

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

(In reply to Jean-Yves Avenard [:jya] from comment #8)
> the code is certainly not inlined:
> 
> http://searchfox.org/mozilla-central/source/mfbt/RefPtr.h#315
> 
> which calls get()
> which isn't inline either.

Compiler can usually inline a member function if it is defined inside the class even without the 'inline' keyword.

However, using 'this' is generally faster because it is usually stored in the register.

Comment 12

[Jean-Yves Avenard [:jya]]

I'm in two mind over it.

While I agree that self does make it slightly more obvious we're in a separate code (though related).

I find that it makes the style painful to read, having code only doing self->mBlah = self->mBlah2 is just annoying to read IMHO.

I'm not strongly opinionated about it however, and as such, I voice my opinion but I don't participate in the r- debate :)

(agree on the performance too, it's unlikely to make a difference, but it's obviously "slower")

Comment 13

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

How about "If |self| needs to appear more than twice in a lambda, consider capturing |this|"?

Comment 14

[Jean-Yves Avenard [:jya]]

sounds like a good compromise..

Comment 15

[James Cheng[:JamesCheng]]

(In reply to Jean-Yves Avenard [:jya] from comment #8)
> the code is certainly not inlined:
> 
> http://searchfox.org/mozilla-central/source/mfbt/RefPtr.h#315
> 
> which calls get()
> which isn't inline either.
> 
> Stepping in into self-> operator, clearly shows of the additional code
> required.
> 
> If the basis for this change was because it was as efficient to dereference
> this vs dereference RefPtr self, then this should be r-

I cannot easily find the standard spec about the inline rule. 

https://stackoverflow.com/a/9192159

I just know `operator->` and `get` are all inside the class definition, so I take them as inlined.

For `&self->mCodec` and `&(self->mCodec)`, I prefer the second one with parentheses(which can easily understand the precedence), but I will follow JW's feedback.

Through all the comments, I don't know if I could land this patch or not....

Could someone help me how to move on?

Thanks.

Comment 16

[James Cheng[:JamesCheng]]

Comment on attachment 8921810 [details]
Bug 1411523 - Remove 'this' from lambda captures [self, this] under dom/media

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

Comment 17

[James Cheng[:JamesCheng]]

Comment on attachment 8921810 [details]
Bug 1411523 - Remove 'this' from lambda captures [self, this] under dom/media

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

Comment 18

[James Cheng[:JamesCheng]]

Comment on attachment 8921810 [details]
Bug 1411523 - Remove 'this' from lambda captures [self, this] under dom/media

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

Comment 19

[Jean-Yves Avenard [:jya]]

It's easy to check in a debugger that the code isn't inlibed

Comment 20

[James Cheng[:JamesCheng]]

I've checked with debugger that I cannot step into operator->

and try runs well.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9efd7be86d1eec43c0845197b2a01e88ff24162&selectedJob=139712895

I've done what I can do. 

For me, I'd like to see explicitly calling member function by `self`.

So I'd like to ask jya's opinion, what should I do next?

Drop this patch or I can land it?


Thank you very much.

Comment 21

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

Check comment 13, the compromise.

Comment 22

[Jean-Yves Avenard [:jya]]

As JW mentioned, check comment 13

Comment 23

[James Cheng[:JamesCheng]]

ok, will do.

but it seems using `self` once is a relatively rare occurrence.

Comment 24

[James Cheng[:JamesCheng]]

Comment on attachment 8921810 [details]
Bug 1411523 - Remove 'this' from lambda captures [self, this] under dom/media

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/192838/diff/4-5/

Comment 25

[James Cheng[:JamesCheng]]

Obey the rule (comment 13)
try looks good with my latest changes.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=de30159729ef709eb76cf3c9ae9e11be59236acf

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/08816a68abd5
Remove 'this' from lambda captures [self, this] under dom/media r=jwwang

Comment 27

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/08816a68abd5