NewRunnable storage default should be StoreCopyPassByConstLRef

RESOLVED FIXED in Firefox 53

Status

()

Core
XPCOM
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: gerald, Assigned: gerald)

Tracking

(Blocks: 1 bug)

49 Branch
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

MozReview Requests

()

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

Attachments

(3 attachments)

(Assignee)

Description

a year ago
When doing something like `NewRunnableMethod<T>`, the default storage for most types is StoreCopyPassByValue:
http://searchfox.org/mozilla-central/rev/efcb1644e49c36445e75d89b434fdf4c84832c84/xpcom/glue/nsThreadUtils.h#740

That is, we keep a copy of the T object in the runnable (good), but then we actually give another copy to the runnable function every time it is run, which seems quite expensive!

Instead we should default to StoreCopyPassByConstLRef, which give a const-reference of the runnable copy.
From there, the function could itself make a copy if it wanted to.

(Thanks to Nathan for reminding me about this.)
(Assignee)

Updated

a year ago
Blocks: 1318230
(Assignee)

Updated

a year ago
Assignee: nobody → gsquelart
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 4

a year ago
mozreview-review
Comment on attachment 8811912 [details]
Bug 1318228 - Replace nsAutoPtr with UniquePtr in GMPCDMProxy -

https://reviewboard.mozilla.org/r/93814/#review93972
Attachment #8811912 - Flags: review?(cpearce) → review+

Comment 5

a year ago
mozreview-review
Comment on attachment 8811912 [details]
Bug 1318228 - Replace nsAutoPtr with UniquePtr in GMPCDMProxy -

https://reviewboard.mozilla.org/r/93814/#review93974

::: dom/media/gmp/GMPCDMProxy.h:122
(Diff revision 1)
>    friend class gmp_InitDoneCallback;
>    friend class gmp_InitGetGMPDecryptorCallback;
>  
>    struct InitData {
>      uint32_t mPromiseId;
>      nsString mOrigin;

In the commit message:
s/GMPCMDProxy/GMPCDMProxy/

I make this typo all the time.

Comment 6

a year ago
mozreview-review
Comment on attachment 8811913 [details]
Bug 1318228 - Replace nsAutoPtr with UniquePtr in MediaDrmCDMProxy -

https://reviewboard.mozilla.org/r/93816/#review93976
Attachment #8811913 - Flags: review?(cpearce) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 9

a year ago
mozreview-review
Comment on attachment 8811911 [details]
Bug 1318228 - NewRunnable defaults to StoreCopyPassByConstLRef -

https://reviewboard.mozilla.org/r/93812/#review94326

This ought to be totally safe.
Attachment #8811911 - Flags: review?(nfroyd) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 13

a year ago
Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4b01174ea634
NewRunnable defaults to StoreCopyPassByConstLRef - r=froydnj
https://hg.mozilla.org/integration/autoland/rev/ace53fd3af96
Replace nsAutoPtr with UniquePtr in GMPCDMProxy - r=cpearce
https://hg.mozilla.org/integration/autoland/rev/8b9935e92099
Replace nsAutoPtr with UniquePtr in MediaDrmCDMProxy - r=cpearce

Comment 14

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4b01174ea634
https://hg.mozilla.org/mozilla-central/rev/ace53fd3af96
https://hg.mozilla.org/mozilla-central/rev/8b9935e92099
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.