1144820 - AddProfileTimelineMarker should use moves instead of calling .release() on a non-const reference

Status: RESOLVED FIXED

(DevTools :: Performance Tools (Profiler/Timeline), defect)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#3073,3076

  void
  nsDocShell::AddProfileTimelineMarker(UniquePtr<TimelineMarker>& aMarker)
  {
    if (mProfileTimelineRecording) {
      mProfileTimelineMarkers.AppendElement(aMarker.release());
  ...

Usually, a reference to a UniquePtr means that it isn't taking ownership. But that is not the case here.

To be clear that AddProfileTimelineMarker is taking ownership, this method should take a `UniquePtr<TimelineMarker>&&` and we should `mozilla::Move` the marker at the callsite. Additionally, if `mProfileTimelineMarkers` was an array of `UniquePtr`s, then we would get clean up For Free as well. (I don't know if nsTArray can hold move-only types, but mozilla::Vector can).

Thanks, Tromey!

Comment 1

[Tom Tromey :tromey]

Attachment: use rvalue reference in AddProfileTimelineMarker

Comment 2

[Tom Tromey :tromey]

Attachment: use nsTArray<UniquePtr<>> to hold timeline markers

I pulled this out to a separate patch because the last time around a
smart-pointer array was not wanted.

Comment 3

[Tom Tromey :tromey]

Attachment: use nsTArray<UniquePtr<>> to hold timeline markers

Sigh, forgot to back out an include from when I thought I'd need
Vector.

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8579590 [details] [diff] [review]
use rvalue reference in AddProfileTimelineMarker

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

Great!

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8579594 [details] [diff] [review]
use nsTArray<UniquePtr<>> to hold timeline markers

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

Yay!

::: docshell/base/nsDocShell.cpp
@@ +2965,3 @@
>  
>    for (uint32_t i = 0; i < mProfileTimelineMarkers.Length(); ++i) {
> +    TimelineMarker* startPayload = mProfileTimelineMarkers[i].get();

Can we get away with a reference to the UniquePtr, so that it is clear that SomeoneElseWillFreeThis (a subclass of SomeoneElsesProblem)?

  UniquePtr<TimelineMarker>& startPayload = ...;

I don't think this would require cascading changes below, because of the -> overloading. If, however, it does require cascading changes, then just ignore this comment.

Same for other .get() uses below.

Comment 6

[Tom Tromey :tromey]

Attachment: use nsTArray<UniquePtr<>> to hold timeline markers

Here's a version that removes the .get()s.  To totally do this I had
to change TimelineMarker::Equals to take a reference, but I think
that's an improvement as well, as it makes it clear that the argument
cannot be null.

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(Did you mean to either carry over the r+ or reflag for review?)

Comment 8

[Tom Tromey :tromey]

(In reply to Nick Fitzgerald [:fitzgen] from comment #7)
> (Did you mean to either carry over the r+ or reflag for review?)

I was going to mark them for smaug to review, as he's reviewed the other
patches in this area.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8579590 [details] [diff] [review]
use rvalue reference in AddProfileTimelineMarker

But you still call release(). Do we need it?

(In general I'm not a fan of Move + &&. Tends to make code harder to read.)

Comment 10

[Tom Tromey :tromey]

(In reply to Olli Pettay [:smaug] from comment #9)
> Comment on attachment 8579590 [details] [diff] [review]
> use rvalue reference in AddProfileTimelineMarker
> 
> But you still call release(). Do we need it?

Yeah, because this first patch doesn't change the type of mProfileTimelineMarkers.
I split that into a separate patch because in some earlier bug a similar idea
was rejected (though it wasn't clear why -- and it wasn't your review).

> (In general I'm not a fan of Move + &&. Tends to make code harder to read.)

I think Nick was surprised that this API took a non-const reference and then mutated it.
Move makes this more clear by requiring the call sites to be explicit.

Comment 11

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Tom Tromey :tromey from comment #10)
> (In reply to Olli Pettay [:smaug] from comment #9)
> > (In general I'm not a fan of Move + &&. Tends to make code harder to read.)
> 
> I think Nick was surprised that this API took a non-const reference and then
> mutated it.
> Move makes this more clear by requiring the call sites to be explicit.

It also makes ownership abundantly clear, which jives really well with UniquePtr in general, which exists to make lifetimes and clean up easier.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8579590 [details] [diff] [review]
use rvalue reference in AddProfileTimelineMarker

What if one passes a marker without Move()?

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

I mean, what does the patch help here? I guess I'm missing something.

Comment 14

[Tom Tromey :tromey]

(In reply to Olli Pettay [:smaug] from comment #13)
> I mean, what does the patch help here? I guess I'm missing something.

If you use a UniquePtr at the call site but leave off the Move, you get
an error.

            docShell->AddProfileTimelineMarker(marker);

=>

In file included from /home/tromey/firefox-git/workdir-rebase-debugger/obj-x86_64-unknown-linux-gnu/dom/base/Unified_cpp_dom_base0.cpp:47:0:
/home/tromey/firefox-git/workdir-rebase-debugger/dom/base/Console.cpp: In member function ‘void mozilla::dom::Console::Method(JSContext*, mozilla::dom::Console::MethodName, const nsAString_internal&, const mozilla::dom::Sequence<JS::Value>&)’:
/home/tromey/firefox-git/workdir-rebase-debugger/dom/base/Console.cpp:1060:54: error: cannot bind ‘mozilla::UniquePtr<TimelineMarker>’ lvalue to ‘mozilla::UniquePtr<TimelineMarker>&&’
             docShell->AddProfileTimelineMarker(marker);
                                                      ^
In file included from /home/tromey/firefox-git/workdir-rebase-debugger/dom/base/Console.cpp:25:0,
                 from /home/tromey/firefox-git/workdir-rebase-debugger/obj-x86_64-unknown-linux-gnu/dom/base/Unified_cpp_dom_base0.cpp:47:
/home/tromey/firefox-git/workdir-rebase-debugger/docshell/base/nsDocShell.h:275:8: error:   initializing argument 1 of ‘void nsDocShell::AddProfileTimelineMarker(mozilla::UniquePtr<TimelineMarker>&&)’
   void AddProfileTimelineMarker(mozilla::UniquePtr<TimelineMarker>&& aMarker);
        ^


The net effect is that the caller must be explicit about the transfer of ownership.

Comment 15

[Tom Tromey :tromey]

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

Comment 16

[Tom Tromey :tromey]

That try run has a failure, but I don't see how it could be caused by this patch.
I'll rebase & try again when I'm back.

Comment 17

[Tom Tromey :tromey]

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

Comment 18

[Tom Tromey :tromey]

Attachment: use rvalue reference in AddProfileTimelineMarker

Rebased.  Carrying over r+.

Comment 19

[Tom Tromey :tromey]

Attachment: use nsTArray<UniquePtr<>> to hold timeline markers

Rebased and updated for MOZ_OVERRIDE removal.
Carrying over r+.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/0af707c2229d
https://hg.mozilla.org/integration/fx-team/rev/d8a86f570ca9

Comment 21

[Ekanan Ketunuti]

https://hg.mozilla.org/mozilla-central/rev/0af707c2229d
https://hg.mozilla.org/mozilla-central/rev/d8a86f570ca9