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

RESOLVED FIXED in Firefox 58

Status

()

Core
Audio/Video: Playback
RESOLVED FIXED
28 days ago
26 days ago

People

(Reporter: JamesCheng, Assigned: JamesCheng)

Tracking

Trunk
mozilla58
Points:
---

Firefox Tracking Flags

(firefox58 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

28 days ago
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 hidden (mozreview-request)
(Assignee)

Updated

28 days ago
No longer blocks: 778617
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 4

28 days ago
mozreview-review
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 6

27 days ago
mozreview-review
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

27 days ago
mozreview-review-reply
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..
(Assignee)

Comment 15

27 days ago
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
It's easy to check in a debugger that the code isn't inlibed
(Assignee)

Comment 20

27 days ago
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)
(Assignee)

Comment 23

27 days ago
ok, will do.

but it seems using `self` once is a relatively rare occurrence.
Comment hidden (mozreview-request)
(Assignee)

Comment 25

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

Comment 26

27 days ago
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
Last Resolved: 26 days ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.