Use movable types for CallStackProfile(s) to remove copying of data.

components/metrics/call_stack_profile_metrics_provider.h

Index: components/metrics/call_stack_profile_metrics_provider.h
diff --git a/components/metrics/call_stack_profile_metrics_provider.h b/components/metrics/call_stack_profile_metrics_provider.h
index d26000be087ba95a7242dd8c945431f2105b9229..ab13d46d126b6930d715e40143c2b8778eafac96 100644
--- a/components/metrics/call_stack_profile_metrics_provider.h
+++ b/components/metrics/call_stack_profile_metrics_provider.h
@@ -35,7 +35,7 @@ class CallStackProfileMetricsProvider : public MetricsProvider {
   static void ReceiveCompletedProfiles(
       const CallStackProfileParams& params,
       base::TimeTicks start_timestamp,
-      const base::StackSamplingProfiler::CallStackProfiles& profiles);
+      base::StackSamplingProfiler::CallStackProfiles profiles);

[Alexei Svitkine (slow), 2016/10/24 15:26:22]

Passing by non-const ref that results in a copy is usually not done in Chromium
- so this code and similarly for  Collect() in the other file ends up looking
like a mistake.

a) What's the reason for doing it this way? Can the copy be done inside the
function explicitly instead rather than implicitly through the function call?
b) Otherwise, if we do want to keep the copy as part of the function call, we
should document that it's done so intentionally.

[bcwhite, 2016/10/24 21:30:30]

CallStackProfiles doesn't allow copy so only std::move is allowed when passing
the parameter.  The CompletedCallback function type, which is what this is,
(stack_sampling_profiler.h:166) documents that these are move-only parameters.

The alternative is to use type&& for the parameter but that's against the style
guide except for (1) move ctor/assign or (2) perfect forwarding.

You could argue that this is a case of #2 but Mojo does its "move-only"
parameters in this way (i.e. no &&) so I've elected to follow that style in
order to have consistency.

[Alexei Svitkine (slow), 2016/10/25 14:34:04]

Sorry, I don't think I'm following. But maybe I don't understand something in
C++.

How can CallStackProfiles which is a std::vector<CallStackProfile> be move only?
Doesn't vector support copy - what's preventing it to be moved?

Even then, if it was move only, I don't understand how that would work when
you're passing the type by value. As far as I understand, moving requires
modifying the original value - but how can a function call modify the original
value (when it's not passed by either pointer or non-const ref)?

For example, in this code:

base::StackSamplingProfiler::CallStackProfiles profiles;
profiles.push_back(...);
ReceiveCompletedProfiles(params, start_timestamp, profiles);

I don't understand how the profiles param can be move only. A move, if I
understand correctly, would transfer the contents of profiles into the param -
but the above code as written shouldn't modify |profiles| local var in any way -
or am I wrong?

Or did you mean the caller has to only call the function with
std::move(profiles) for the param? If so, that's not what I think of - when
reading "move only" - since if the caller doesn't do that, it still would end up
copying - and so if that's the intention it should be clearly documented.

But perhaps I'm missing something?

[bcwhite, 2016/10/25 15:01:04]

std::vector supports both copy and move construction.  If the latter, it just
moves some pointers around and steals the data from its parameter.
All true.  If it was const, it couldn't be modified at all.  Passed by value,
the original can't be modified.

That means that when calling this, the caller has to explicitly specify
std::move(var) in the parameter list.  That moves the contents into the
parameter which is then moved into its final destination.

This, incidentally, is why doing an rvalue-reference (i.e. type&&) would be
preferred since it just as lightweight as a regular reference... but is against
the style guide and would conflict with the code generated by mojo.
You're correct.  It has to be

ReceiveCompletedProfiles(params, start_timestamp, std::move(profiles));
It's always the way it has to be done.  There's no implicit moves allowed where
there is any possibility that the variable could be referenced again further
down -- that basically means that if it has a name (rather than the result of an
expression), then std::move() is required to pass to a move-only (i.e. has move
constructor but no copy constructor) parameter.

std::unique_ptr is the best example of this.  You can never pass it as a
parameter without an explicit std::move.

[Alexei Svitkine (slow), 2016/10/25 15:21:18]

OK, so given the above, please add a comment to each function where you have
such a parameter that mentions that it's passed by value so that the caller can
use std::move() to avoid copies.

This way, having such a comment makes it explicit that it's intentional that
it's being passed by value (and not a mistake of someone forgetting to put
const&).

[bcwhite, 2016/10/25 17:16:21]

Done.

 
   // MetricsProvider:
   void OnRecordingEnabled() override;