1185759 - Use (move) assignment instead of nsTArray::MoveElementsFrom

Status: NEW

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

Description

[Biru [:poiru]]

A couple places in dom/media/ confusingly use MoveElementsFrom (which is a deceptive name, to be fair) instead of assignment.

Comment 1

[Biru [:poiru]]

Attachment: Use move assignment instead of nsTArray::MoveElementsFrom in Intervals.h

Comment 2

[Biru [:poiru]]

Attachment: Use move assignment instead of nsTArray::MoveElementsFrom in {MediaManager,MediaEngineCameraVideoSource}.cpp

Comment 3

[Jean-Yves Avenard [:jya]]

Comment on attachment 8636287 [details] [diff] [review]
Use move assignment instead of nsTArray::MoveElementsFrom in Intervals.h

Review of attachment 8636287 [details] [diff] [review]:
-----------------------------------------------------------------

I'd like to know how MoveElementsFrom is actually mis-used ; it certainly doesn't look that way to me and the resulting new code ends up doing more delete / new and copying rather than moving.

TimeIntervals is now used in lots of critical loops; accounting for a significant % of CPU time.

::: dom/media/Intervals.h
@@ +267,5 @@
>    {
>    }
>  
>    IntervalSet(SelfType&& aOther)
> +    : mIntervals(Move(aOther.mIntervals))

From what I gathered at the time, and I don't see any new changes that would affect this decision:

There are no rvalue constructors for nsTArray ; as such this code will end up doing a full copy instead.

While mIntervals.MoveElementsFrom(Move(aOther.mIntervals)); will actually move the items.

Hence it's far more efficient to do so.

@@ +377,5 @@
>      }
>      for (; i < mIntervals.Length(); i++) {
>        normalized.AppendElement(Move(mIntervals[i]));
>      }
> +    mIntervals = normalized;

Same here, now this will call the mIntervals destructor, and reallocate a new one, and then copy the elements from normalized.

While the previous code, avoid the destructor (saving a delete / new) and then move the elements from the now unused array to mIntervals.

Comment 4

[Biru [:poiru]]

Attachment: Use move assignment instead of nsTArray::MoveElementsFrom in Intervals.h

(In reply to Jean-Yves Avenard [:jya] from comment #3)
> Comment on attachment 8636287 [details] [diff] [review]
> Use move assignment instead of nsTArray::MoveElementsFrom in Intervals.h
> 
> Review of attachment 8636287 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'd like to know how MoveElementsFrom is actually mis-used ; it certainly
> doesn't look that way to me and the resulting new code ends up doing more
> delete / new and copying rather than moving.

MoveElementsFrom is deceptively named (see bug 1185763) and doesn't do quite what you would expect based on the name. First of all, it does *not* move elements and has nothing to do with move semantics. Instead, it *appends* elements by memcpying the bits from another array. This is much slower than a real move.

When you want to *append* elements than can be safely memcpy'd, MoveElementsFrom can be faster than using a bare AppendElements. When you want to *assign* an array of elements (like we are here), MoveElementsFrom is much slower than the move assignment operator and move constructor. In the best case, MoveElementsFrom has to memcpy the bits of all elements whereas the move assignment operator and move constructor can just swap a pointer.

> There are no rvalue constructors for nsTArray ; as such this code will end
> up doing a full copy instead.

There is, see: http://hg.mozilla.org/mozilla-central/annotate/d00e4167b482/xpcom/glue/nsTArray.h#l838
 
> While mIntervals.MoveElementsFrom(Move(aOther.mIntervals)); will actually
> move the items.
> 
> Hence it's far more efficient to do so.

This is not correct, see above.

> @@ +377,5 @@
> >      }
> >      for (; i < mIntervals.Length(); i++) {
> >        normalized.AppendElement(Move(mIntervals[i]));
> >      }
> > +    mIntervals = normalized;
> 
> Same here, now this will call the mIntervals destructor, and reallocate a
> new one, and then copy the elements from normalized.

Sorry, this was a mistake/typo. I meant that to be `Move(normalized)` like it was in the other cases. Fixed now.

Comment 5

[Jean-Yves Avenard [:jya]]

(In reply to Birunthan Mohanathas [:poiru] from comment #4)
> Created attachment 8636327 [details] [diff] [review]
> Use move assignment instead of nsTArray::MoveElementsFrom in Intervals.h
> 
> (In reply to Jean-Yves Avenard [:jya] from comment #3)
> > Comment on attachment 8636287 [details] [diff] [review]
> > Use move assignment instead of nsTArray::MoveElementsFrom in Intervals.h
> > 
> > Review of attachment 8636287 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I'd like to know how MoveElementsFrom is actually mis-used ; it certainly
> > doesn't look that way to me and the resulting new code ends up doing more
> > delete / new and copying rather than moving.
> 
> MoveElementsFrom is deceptively named (see bug 1185763) and doesn't do quite
> what you would expect based on the name. First of all, it does *not* move
> elements and has nothing to do with move semantics. Instead, it *appends*
> elements by memcpying the bits from another array. This is much slower than
> a real move.

My first implementation using Move (like you do in this patch), I found that we *never* entered the rvalue copy operator.

It's very easy to test. Set a breakpoint on mIntervals = Move(normalized) ; and that get into the default copy (it certainly did when I last tried it)

Comment 6

[Biru [:poiru]]

(In reply to Jean-Yves Avenard [:jya] from comment #5)
> (In reply to Birunthan Mohanathas [:poiru] from comment #4)
> > Created attachment 8636327 [details] [diff] [review]
> > Use move assignment instead of nsTArray::MoveElementsFrom in Intervals.h
> > 
> > (In reply to Jean-Yves Avenard [:jya] from comment #3)
> > > Comment on attachment 8636287 [details] [diff] [review]
> > > Use move assignment instead of nsTArray::MoveElementsFrom in Intervals.h
> > > 
> > > Review of attachment 8636287 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > I'd like to know how MoveElementsFrom is actually mis-used ; it certainly
> > > doesn't look that way to me and the resulting new code ends up doing more
> > > delete / new and copying rather than moving.
> > 
> > MoveElementsFrom is deceptively named (see bug 1185763) and doesn't do quite
> > what you would expect based on the name. First of all, it does *not* move
> > elements and has nothing to do with move semantics. Instead, it *appends*
> > elements by memcpying the bits from another array. This is much slower than
> > a real move.
> 
> My first implementation using Move (like you do in this patch), I found that
> we *never* entered the rvalue copy operator.
> 
> It's very easy to test. Set a breakpoint on mIntervals = Move(normalized) ;
> and that get into the default copy (it certainly did when I last tried it)

You are absolutely correct. It seems like the move assignment operator is missing for nsAutoTArray. I filed bug 1185794 to correct this oversight.

Comment 7

[Maire Reavy [:mreavy]]

Hi Birunthan  -- Is this ready to land (module addressing bitrot)?

Comment 8

[Biru [:poiru]]

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #7)
> Hi Birunthan  -- Is this ready to land (module addressing bitrot)?

Nope, not yet. I'll try to get this in landable shape soon. Sorry for the delayed response.

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.