tracing: memory-infra cleanup post-C++11 + test coverage

tracing: memory-infra cleanup post-C++11 + test coverage

In the context of a bigger refactoring I realized that:
 - ProcessMemoryDump memory management can be simplified now
   that we have C++11 moving semantics.
 - A heap-profiler code path was untested.
 - Using SmallMap in our big memory dumps it's like drinking diet
   lemonade with deep fried fish and chips.
Also taking the occasion for some minor cleanups and clang-format.

Committed: https://crrev.com/fbc697860e2d5e4271990b90ae29a8b1c59417ad
Cr-Commit-Position: refs/heads/master@{#376750}

[Primiano Tucci (use gerrit), 2016-02-22 13:03:07]

Description was changed from

==========
tracing: memory-infra cleanup post-C++11 plus test coverage

In the context of a bigger refactoring I realized that:
 - ProcessMemoryDump memory managedment can be simplified now
   that we have C++11 moving semantics.
 - A heap-profiler code path was untested.
Also taking the occasion for some minor cleanups and clang-format.

BUG=
==========

to

==========
tracing: memory-infra cleanup post-C++11 plus test coverage

In the context of a bigger refactoring I realized that:
 - ProcessMemoryDump memory managedment can be simplified now
   that we have C++11 moving semantics.
 - A heap-profiler code path was untested.
 - using SmallMap in our big memory dumps it's like putting dietetic lemon 
    on a deep-fried fish and chips.
Also taking the occasion for some minor cleanups and clang-format.

BUG=
==========

[Primiano Tucci (use gerrit), 2016-02-22 13:03:07]

primiano@chromium.org changed reviewers:
+ petrcermak@chromium.org, ssid@chromium.org

[Primiano Tucci (use gerrit), 2016-02-22 13:03:26]

PTAL. cleanup. Feel free to tick if LG.

[Primiano Tucci (use gerrit), 2016-02-22 13:03:54]

Description was changed from

==========
tracing: memory-infra cleanup post-C++11 plus test coverage

In the context of a bigger refactoring I realized that:
 - ProcessMemoryDump memory managedment can be simplified now
   that we have C++11 moving semantics.
 - A heap-profiler code path was untested.
 - using SmallMap in our big memory dumps it's like putting dietetic lemon 
    on a deep-fried fish and chips.
Also taking the occasion for some minor cleanups and clang-format.

BUG=
==========

to

==========
tracing: memory-infra cleanup post-C++11 plus test coverage

In the context of a bigger refactoring I realized that:
 - ProcessMemoryDump memory managedment can be simplified now
   that we have C++11 moving semantics.
 - A heap-profiler code path was untested.
 - using SmallMap in our big memory dumps it's like putting dietetic lemon 
   on a deep-fried fish and chips.
Also taking the occasion for some minor cleanups and clang-format.

BUG=
==========

[petrcermak, 2016-02-22 13:33:25]

LGTM with some comments about the usage of move operators and some description
nits (sorry):

* s/managedment/management/
* s/using/Using/ (so that all list items begin with a capital letter)
* s/it's/is/ ("using SmallMap ... dumps" is the subject of the sentence)
* s/a deep-fried/deep fried/ (pseudoplural)
* Unless you know that "dietetic lemon" is a common expression, I'd go for "diet
lemon" (Google search "diet lemonade" > "dietary lemonade" > "dietetic
lemonade"; FYI, I searched for "lemonade" because "lemon" is very often an
adjective).
* -1 space on the line "on a deep-fried ..." (to be consistent with the line
"that we have C+11 ...")
* Remove the "BUG=" line (or provide a bug number)

Thanks,
Petr

https://codereview.chromium.org/1718173002/diff/1/base/trace_event/process_me...
File base/trace_event/process_memory_dump.cc (right):

https://codereview.chromium.org/1718173002/diff/1/base/trace_event/process_me...
base/trace_event/process_memory_dump.cc:147:
std::make_pair(mad->absolute_name(), std::move(mad)));
Why don't you change the signature of this method to
'scoped_ptr<MemoryAllocatorDump> &&' instead? That should be safer because the
compiler would check that you always call it with rvalues.

https://codereview.chromium.org/1718173002/diff/1/base/trace_event/process_me...
base/trace_event/process_memory_dump.cc:221:
AddAllocatorDumpInternal(std::move(it.second));
Note that if you keep the std::move conversion on line 147, you'll be moving
this value twice. I don't think you need that.

https://codereview.chromium.org/1718173002/diff/1/base/trace_event/process_me...
File base/trace_event/process_memory_dump.h (right):

https://codereview.chromium.org/1718173002/diff/1/base/trace_event/process_me...
base/trace_event/process_memory_dump.h:14: #include
"base/containers/hash_tables.h"
Do you still need this include?

https://codereview.chromium.org/1718173002/diff/1/base/trace_event/process_me...
File base/trace_event/process_memory_dump_unittest.cc (right):

https://codereview.chromium.org/1718173002/diff/1/base/trace_event/process_me...
base/trace_event/process_memory_dump_unittest.cc:21: return (it ==
pmd.heap_dumps().end()) ? nullptr : it->second.get();
nit: no need for parentheses

[Primiano Tucci (use gerrit), 2016-02-22 15:50:42]

Description was changed from

==========
tracing: memory-infra cleanup post-C++11 plus test coverage

In the context of a bigger refactoring I realized that:
 - ProcessMemoryDump memory managedment can be simplified now
   that we have C++11 moving semantics.
 - A heap-profiler code path was untested.
 - using SmallMap in our big memory dumps it's like putting dietetic lemon 
   on a deep-fried fish and chips.
Also taking the occasion for some minor cleanups and clang-format.

BUG=
==========

to

==========
tracing: memory-infra cleanup post-C++11 + test coverage

In the context of a bigger refactoring I realized that:
 - ProcessMemoryDump memory management can be simplified now
   that we have C++11 moving semantics.
 - A heap-profiler code path was untested.
 - Using SmallMap in our big memory dumps it's like drinking diet 
   lemonade with deep fried fish and chips.
Also taking the occasion for some minor cleanups and clang-format.
==========

[Primiano Tucci (use gerrit), 2016-02-22 15:56:07]

Thanks

https://codereview.chromium.org/1718173002/diff/1/base/trace_event/process_me...
File base/trace_event/process_memory_dump.cc (right):

https://codereview.chromium.org/1718173002/diff/1/base/trace_event/process_me...
base/trace_event/process_memory_dump.cc:147:
std::make_pair(mad->absolute_name(), std::move(mad)));
On 2016/02/22 13:33:25, petrcermak wrote:
> Why don't you change the signature of this method to
> 'scoped_ptr<MemoryAllocatorDump> &&' instead? That should be safer because the
> compiler would check that you always call it with rvalues.

There doesn't seem to be any precedent for this. Also scoped_ptr is a move-only
type.

https://codereview.chromium.org/1718173002/diff/1/base/trace_event/process_me...
base/trace_event/process_memory_dump.cc:221:
AddAllocatorDumpInternal(std::move(it.second));
On 2016/02/22 13:33:25, petrcermak wrote:
> Note that if you keep the std::move conversion on line 147, you'll be moving
> this value twice. I don't think you need that.

See reply above, no precedent.
Even if that should happen, amen, that's not a fast-path.
But my hope about all these move semantic was that the compiler was smart enough
to elide the unnecessary paths.
Don't have time to check right now, but very likely the entire
AddAllocatorDumpInternal gets inlined.

https://codereview.chromium.org/1718173002/diff/1/base/trace_event/process_me...
File base/trace_event/process_memory_dump.h (right):

https://codereview.chromium.org/1718173002/diff/1/base/trace_event/process_me...
base/trace_event/process_memory_dump.h:14: #include
"base/containers/hash_tables.h"
On 2016/02/22 13:33:25, petrcermak wrote:
> Do you still need this include?

Good spot. Removed.

https://codereview.chromium.org/1718173002/diff/1/base/trace_event/process_me...
File base/trace_event/process_memory_dump_unittest.cc (right):

https://codereview.chromium.org/1718173002/diff/1/base/trace_event/process_me...
base/trace_event/process_memory_dump_unittest.cc:21: return (it ==
pmd.heap_dumps().end()) ? nullptr : it->second.get();
On 2016/02/22 13:33:25, petrcermak wrote:
> nit: no need for parentheses

Done.

[Primiano Tucci (use gerrit), 2016-02-22 15:56:41]

The CQ bit was checked by primiano@chromium.org

[Primiano Tucci (use gerrit), 2016-02-22 15:56:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
petrcermak@chromium.org
Link to the patchset: https://codereview.chromium.org/1718173002/#ps20001
(title: "Re petrcermak review")

[commit-bot: I haz the power, 2016-02-22 15:56:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1718173002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1718173002/20001

[commit-bot: I haz the power, 2016-02-22 17:23:43]

Description was changed from

==========
tracing: memory-infra cleanup post-C++11 + test coverage

In the context of a bigger refactoring I realized that:
 - ProcessMemoryDump memory management can be simplified now
   that we have C++11 moving semantics.
 - A heap-profiler code path was untested.
 - Using SmallMap in our big memory dumps it's like drinking diet 
   lemonade with deep fried fish and chips.
Also taking the occasion for some minor cleanups and clang-format.
==========

to

==========
tracing: memory-infra cleanup post-C++11 + test coverage

In the context of a bigger refactoring I realized that:
 - ProcessMemoryDump memory management can be simplified now
   that we have C++11 moving semantics.
 - A heap-profiler code path was untested.
 - Using SmallMap in our big memory dumps it's like drinking diet 
   lemonade with deep fried fish and chips.
Also taking the occasion for some minor cleanups and clang-format.
==========

[commit-bot: I haz the power, 2016-02-22 17:23:44]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-02-22 17:24:45]

Description was changed from

==========
tracing: memory-infra cleanup post-C++11 + test coverage

In the context of a bigger refactoring I realized that:
 - ProcessMemoryDump memory management can be simplified now
   that we have C++11 moving semantics.
 - A heap-profiler code path was untested.
 - Using SmallMap in our big memory dumps it's like drinking diet 
   lemonade with deep fried fish and chips.
Also taking the occasion for some minor cleanups and clang-format.
==========

to

==========
tracing: memory-infra cleanup post-C++11 + test coverage

In the context of a bigger refactoring I realized that:
 - ProcessMemoryDump memory management can be simplified now
   that we have C++11 moving semantics.
 - A heap-profiler code path was untested.
 - Using SmallMap in our big memory dumps it's like drinking diet
   lemonade with deep fried fish and chips.
Also taking the occasion for some minor cleanups and clang-format.

Committed: https://crrev.com/fbc697860e2d5e4271990b90ae29a8b1c59417ad
Cr-Commit-Position: refs/heads/master@{#376750}
==========

[commit-bot: I haz the power, 2016-02-22 17:24:46]

Patchset 2 (id:??) landed as
https://crrev.com/fbc697860e2d5e4271990b90ae29a8b1c59417ad
Cr-Commit-Position: refs/heads/master@{#376750}