Closed Bug 1411523 Opened 2 years ago Closed 2 years ago

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

Categories

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

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: JamesCheng, Assigned: JamesCheng)

Details

Attachments

(1 file)

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.
No longer blocks: MSE
I think |this| is is captured to save typing. E.g. self->DoSomething() v.s DoSomething().
Attachment #8921810 - Flags: review?(gsquelart) → review?(jwwang)
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 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.
Attachment #8921810 - Flags: review?(jwwang) → review+
(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 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 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...
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 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.
(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. :)
(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.
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")
How about "If |self| needs to appear more than twice in a lambda, consider capturing |this|"?
sounds like a good compromise..
(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.
It's easy to check in a debugger that the code isn't inlibed
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.
Flags: needinfo?(jyavenard)
Check comment 13, the compromise.
As JW mentioned, check comment 13
Flags: needinfo?(jyavenard)
ok, will do.

but it seems using `self` once is a relatively rare occurrence.
Pushed by jacheng@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/08816a68abd5
Remove 'this' from lambda captures [self, this] under dom/media r=jwwang
https://hg.mozilla.org/mozilla-central/rev/08816a68abd5
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.