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

components/metrics/call_stack_profile_metrics_provider.h

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef COMPONENTS_METRICS_CALL_STACK_PROFILE_METRICS_PROVIDER_H_
 #define COMPONENTS_METRICS_CALL_STACK_PROFILE_METRICS_PROVIDER_H_
 
 #include <vector>
 
 #include "base/macros.h"
@@ skipping 17 matching lines @@
   // StackSamplingProfilers. This function may be called on any thread.
   static base::StackSamplingProfiler::CompletedCallback GetProfilerCallback(
       const CallStackProfileParams& params);
 
   // Provides completed stack profiles to the metrics provider. Intended for use
   // when receiving profiles over IPC. In-process StackSamplingProfiler users
   // should use GetProfilerCallback() instead.
   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;
   void OnRecordingDisabled() override;
   void ProvideGeneralMetrics(ChromeUserMetricsExtension* uma_proto) override;
 
  protected:
   // Finch field trial and group for reporting profiles. Provided here for test
   // use.
   static const char kFieldTrialName[];
   static const char kReportProfilesGroupName[];
 
   // Reset the static state to the defaults after startup.
   static void ResetStaticStateForTesting();
 
  private:
   // Returns true if reporting of profiles is enabled according to the
   // controlling Finch field trial.
   static bool IsReportingEnabledByFieldTrial();
 
   DISALLOW_COPY_AND_ASSIGN(CallStackProfileMetricsProvider);
 };
 
 }  // namespace metrics
 
 #endif  // COMPONENTS_METRICS_CALL_STACK_PROFILE_METRICS_PROVIDER_H_