Closed Bug 1290036 Opened 3 years ago Closed 3 years ago

Make CreateDecoderParams get rid of unnecessary copy/move

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: JamesCheng, Assigned: JamesCheng)

Details

Attachments

(1 file, 1 obsolete file)

I think CreateDecoderParams can use universal reference to reduce the unnecessary copy or move.

https://dxr.mozilla.org/mozilla-central/rev/db3ed1fdbbeaf5ab1e8fe454780146e7499be3db/dom/media/platforms/PlatformDecoderModule.h#42
Hi Gerald,

Could you please review this patch?

I use this approach to invoke the Set function instead of the original one.

Is it OK or there are some special reason to use the original approach?

Thank you.
Attachment #8775517 - Flags: review?(gsquelart)
Attachment #8775517 - Attachment is obsolete: true
Attachment #8775517 - Flags: review?(gsquelart)
Comment on attachment 8775529 [details]
Bug 1290036 - Make CreateDecoderParams get rid of unnecessary copy/move.

https://reviewboard.mozilla.org/r/67710/#review64790

The first version of this struct contained only pointers and enums, so moving wouldn't have helped; but I see there is a RefPtr now.

So I am happy with forwarding references (formerly known as universal references) to avoid copies where possible. Thank you for working on this.
I'd r+ just that.

However in your patch you have also changed the templated methods to use recursion, ending with an empty Set() -- why this change?
I believe the initial patch was a bit more focussed and efficient by using a single method (per template signature) calling all appropriate Set's at once. (In a little experiment, a 4-argument call produced twice as many asm lines with your new code.)

Though it is quite possible that the end result is the same after compiler optimization, I would need to be convinced that these changes improve the code before I can approve them.
Attachment #8775529 - Flags: review?(gsquelart)
Oh I didn't see that you had cleared the review flag, please disregard my comments if they don't apply anymore.
Hi Gerald,

Sorry that originally I did not use reviewboard for reviewing so the "r cancel" is from that.

You mean that the recursive version may produce more lines of asm instruction lines?

Does "more asm lines" equal to slower performance? 

Intuitively, I think recursive version increase readability though the original version is very cool.

If readability is not convincible, I can modify this page only apply the universal reference.

Thank you~
Flags: needinfo?(gsquelart)
(In reply to Gerald Squelart [:gerald] from comment #3)
> Comment on attachment 8775529 [details]
> Bug 1290036 - Make CreateDecoderParams get rid of unnecessary copy/move.
> 
> https://reviewboard.mozilla.org/r/67710/#review64790
> 
> The first version of this struct contained only pointers and enums, so
> moving wouldn't have helped; but I see there is a RefPtr now.
> 
> So I am happy with forwarding references (formerly known as universal
> references) to avoid copies where possible. Thank you for working on this.
> I'd r+ just that.
> 
> However in your patch you have also changed the templated methods to use
> recursion, ending with an empty Set() -- why this change?

  template <typename... Args>
  CreateDecoderParams(const TrackInfo& aConfig, Args&&... args)
    : mConfig(aConfig)
  {
    Set(mozilla::Forward<Args>(args)...);// Handle sizeof...(Args) == 0 case  
  } 
  I can also use 
  if( sizeof...(Args) != 0 ) Set(...); but I think use a empty Set() is more elegant.
I suppose you're right, readability is important, and in this case it is not critical code and therefore doesn't need "very cool" code.
(Though it could be argued that during debugging, my version would produce simpler error messages than yours -- but hopefully debugging won't be needed!)

One thing you may still want to consider though:
If instead of 'template <typename... Args>', you used 'template <typename T1, typename... Args>' for the constructor (like I had), you would ensure that there is at least one argument for this call.
And for the templated Set method, 'template <typename T1, typename T2, typename... Args> would ensure there are at least two arguments.
With these, I think you could remove the empty Set() method, and also make it more obvious to the human reader which method will be chosen for certain argument lists; while still using the simpler-to-understand recursion.
What do you think?
Flags: needinfo?(gsquelart)
I think the 'expander' trick is simpler and more readable. Recursion needs a twisted mind and is harder to comprehend.
Comment on attachment 8775529 [details]
Bug 1290036 - Make CreateDecoderParams get rid of unnecessary copy/move.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67710/diff/1-2/
Attachment #8775529 - Flags: review?(gsquelart)
Thanks for your suggestion,

I learned something from your comment .

Please review the new patch 

Thank you
Comment on attachment 8775529 [details]
Bug 1290036 - Make CreateDecoderParams get rid of unnecessary copy/move.

https://reviewboard.mozilla.org/r/67710/#review64968

r+ after you remove the log, and assuming your patch actually compiles (i.e., the error log doesn't refer to this version).
I'd recommend running a small try on all platforms to ensure there is no compiler issue anywhere and the media code still works, e.g.: "try: -b do -p all -u mochitest-media,mochitest-media-e10s -t none".

::: log.log:1
(Diff revision 2)
> + 0:05.01 /usr/bin/make -f client.mk MOZ_PARALLEL_BUILD=64 -s

Remove this whole file from the patch.
Attachment #8775529 - Flags: review?(gsquelart) → review+
Comment on attachment 8775529 [details]
Bug 1290036 - Make CreateDecoderParams get rid of unnecessary copy/move.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67710/diff/2-3/
Thanks for addressing. 

Carry r+ and attach treeherder result.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=01fbb07357e9
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9d6defe4416
Make CreateDecoderParams get rid of unnecessary copy/move. r=gerald
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a9d6defe4416
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.