1278772 - Use value semantics to facilitate compiler optimization

Status: RESOLVED FIXED

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

Description

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

See bug 1268314 for the reason.

Comment 1

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

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=faa6c1578dd8

Comment 2

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

Attachment: Bug 1278772 - Use value semantics to facilitate compiler optimization. .

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

Comment 3

[Jean-Yves Avenard [:jya]]

https://reviewboard.mozilla.org/r/58436/#review55294

::: dom/media/MediaEventSource.h:100
(Diff revision 1)
>  template <typename T> struct EventTarget;
>  
>  template <>
>  struct EventTarget<nsIEventTarget> {
>    static void
> -  Dispatch(nsIEventTarget* aTarget, already_AddRefed<nsIRunnable>&& aTask) {
> +  Dispatch(nsIEventTarget* aTarget, already_AddRefed<nsIRunnable> aTask) {

Wasn't this discussed not long ago on m.d.p?

IIRC, the conclusion was that using rvalue was the right thing to do to indicate transfer of ownership.

this isn't happening here.

Comment 4

[Jean-Yves Avenard [:jya]]

https://reviewboard.mozilla.org/r/58436/#review55294

> Wasn't this discussed not long ago on m.d.p?
> 
> IIRC, the conclusion was that using rvalue was the right thing to do to indicate transfer of ownership.
> 
> this isn't happening here.

Found the thread, and the discussion was about conditional transfer of ownership, and indeed the recommendation was to not use rvalue; how is this still applicable here which isn't conditional move (it is always moved)

Comment 5

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

See bug 1268314. The point is not about conditional move, but about compiler optimization.

Comment 6

[Matthew Gregan [:kinetik]]

Comment on attachment 8761080 [details]
Bug 1278772 - Use value semantics to facilitate compiler optimization. .

https://reviewboard.mozilla.org/r/58436/#review55520

I think I need a more detailed explanation or you should have someone is more familiar with C++ move semantics review this.  It looks like this makes it ambiguous whether the value is moved, which would be bad for ease of understanding the code.

Comment 7

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

(In reply to Matthew Gregan [:kinetik] from comment #6)
> It looks like this makes
> it ambiguous whether the value is moved, which would be bad for ease of
> understanding the code.

No, it makes it more obvious to know whether the ownership is transferred or not. Consider the code below:

void foo(UniquePtr<T>&& aVal) {
  if (blah) {
    UniquePtr<T> v = Move(aVal);
  }
}

void bar() {
  UniquePtr<T> v = MakeUnique(...);
  foo(Move(v)); // is the ownership transferred or not?
}

========================================================

void foo2(UniquePtr<T> aVal) {
  if (blah) {
    UniquePtr<T> v = Move(aVal);
  }
}

void bar2() {
  UniquePtr<T> v = MakeUnique(...);
  foo(Move(v)); // the ownership is definitely transferred!
}

========================================================

However, for a already_AddRefed<T>, it doesn't really matter if the ownership is transferred or not because the ownership is shared. It is more about the reason described in bug 1268314.

I will split the patch and add more comments to make the intention more obvious.

Comment 8

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

Comment on attachment 8761080 [details]
Bug 1278772 - Use value semantics to facilitate compiler optimization. .

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

Comment 9

[Matthew Gregan [:kinetik]]

(In reply to JW Wang [:jwwang] from comment #7)
> No, it makes it more obvious to know whether the ownership is transferred or
> not. Consider the code below:

Right, it absolutely improves that situation.  I was concerned about something slightly different, but my wording was ambiguous.  In the original patch, there were places you removed Move(), and I meant that it becomes unclear in those cases that the expectation/intention is still to move the value.

Comment 10

[Matthew Gregan [:kinetik]]

Comment on attachment 8761080 [details]
Bug 1278772 - Use value semantics to facilitate compiler optimization. .

https://reviewboard.mozilla.org/r/58436/#review56244

Comment 11

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

Thanks! I will split other changes into another bug to make it clear.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/21a4cfe06146
Use value semantics to facilitate compiler optimization. r=kinetik.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/21a4cfe06146