[tracing] Implement composable memory usage estimators.

[tracing] Implement composable memory usage estimators.

When writing MemoryDumpProviders it's sometimes necessary to know how
much memory a particular class is responsible for. The answer is not easy
when class' memory usage is spread across several members which use STL
containers.

This CL introduces EstimateMemoryUsage() family of methods to accurately
estimate memory usage of STL containers. I.e. if EstimateMemoryUsage() is
implemented for type T, the memory usage of std::map<int, std::vector<T>>
is just one EstimateMemoryUsage() call away (instead of two nested loops),
and also includes memory used by std::map/vector (which can be big!).

The change is needed for sync MDP provider, but can also be used in other
MDPs. As an example this CL converts "ui/resource_manager" MDP to use
estimators resulting in cleaner code.

BUG=649065
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

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

[Dmitry Skiba, 2016-10-24 08:26:11]

Description was changed from

==========
[tracing] Implement composable memory usage estimators.

When writing MemoryDumpProviders it's sometimes necessary to know how
much memory a particular class is responsible for. The answer is not easy
when class' memory usage is spread across several members which use STL
containers.

This CL introduces EstimateMemoryUsage() family of methods to accurately
estimate memory usage of STL containers. I.e. if EstimateMemoryUsage() is
implemented for type T, the memory usage of std::map<int, std::vector<T>>
is just one EstimateMemoryUsage() call away (instead of two nested loops),
and also includes memory used by std::map/vector (which can be big!).

The change is needed for sync MDP provider, but can also be used in other
MDPs. As an example this CL converts "ui/resource_manager" MDP to use
estimators resulting in cleaner code.

BUG=649065
==========

to

==========
[tracing] Implement composable memory usage estimators.

When writing MemoryDumpProviders it's sometimes necessary to know how
much memory a particular class is responsible for. The answer is not easy
when class' memory usage is spread across several members which use STL
containers.

This CL introduces EstimateMemoryUsage() family of methods to accurately
estimate memory usage of STL containers. I.e. if EstimateMemoryUsage() is
implemented for type T, the memory usage of std::map<int, std::vector<T>>
is just one EstimateMemoryUsage() call away (instead of two nested loops),
and also includes memory used by std::map/vector (which can be big!).

The change is needed for sync MDP provider, but can also be used in other
MDPs. As an example this CL converts "ui/resource_manager" MDP to use
estimators resulting in cleaner code.

BUG=649065
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[Dmitry Skiba, 2016-10-24 08:28:06]

dskiba@google.com changed reviewers:
+ primiano@chromium.org

[Primiano Tucci (use gerrit), 2016-10-24 15:03:53]

On 2016/10/24 08:28:07, Dmitry Skiba wrote:

Ok wait this needs some context.
What is the use case of this? heap profiler? This is throwing a 1k CL out of the
blue.
More importantly, this scoped_usage seems very simillar to what siggi@
introduced in https://codereview.chromium.org/2427503003/ and prior CLs.
Can you:
 - explain me what you are trying to achieve?
 - check if siggi's thing can be useful here?

[Dmitry Skiba, 2016-10-24 17:13:40]

On 2016/10/24 15:03:53, Primiano Tucci wrote:
> On 2016/10/24 08:28:07, Dmitry Skiba wrote:
> 
> Ok wait this needs some context.
> What is the use case of this? heap profiler? This is throwing a 1k CL out of
the
> blue.
> More importantly, this scoped_usage seems very simillar to what siggi@
> introduced in https://codereview.chromium.org/2427503003/ and prior CLs.
> Can you:
>  - explain me what you are trying to achieve?
>  - check if siggi's thing can be useful here?

This is for writing MemoryDumpProviders. If you have a class that stores stuff
in STL containers, you have to iterate over all of them to calculate memory
usage. And sometimes it's not just a single container, but something like
std::unordered_map<int, std::unique_ptr<T>>[10]. With EstimateMemoryUsage() you
just call one function and voila.

Compare Directory::GetApproximateMemoryUsage() here
https://codereview.chromium.org/2084243004/patch/170001/180011 with
Directory::OnMemoryDump() here
https://codereview.chromium.org/2382443006/patch/1/10018 and
EntryKernel::EstimateMemoryUsage()
https://codereview.chromium.org/2382443006/patch/1/10022

Another example in this CL:
https://codereview.chromium.org/2440393002/patch/1/10014

I tried using siggi's ScopedThreadHeapUsage, but it's not accurate enough
because it uses AllocatorDispatch::get_size_estimate_function. ScopedMemoryUsage
uses hash map and returns exact answer. I'll add some comments to it.

[Primiano Tucci (use gerrit), 2016-10-24 17:23:10]

There are two fundamental things I don't understand here:
1) This CL introduces a ScopedMemoryUsage and the STL estimator. They seem quite
orthogonal and ScopedMemoryUsage seems to be used only by the test. Did you
accidentally pull in too many files maybe?
2) Even just focusing on the Estimator. When I look at resource_manager_impl.cc
it seems to me that you are getting rid of a very readable 30-lines loop, and
replacing that with an invocation of a file which has 500 lines of templates and
has references to internals of STL implementation.
How many other cases do we have where this will be used roughly?

[Dmitry Skiba, 2016-10-24 17:57:23]

On 2016/10/24 17:23:10, Primiano Tucci wrote:
> There are two fundamental things I don't understand here:
> 1) This CL introduces a ScopedMemoryUsage and the STL estimator. They seem
quite
> orthogonal and ScopedMemoryUsage seems to be used only by the test. Did you
> accidentally pull in too many files maybe?
> 2) Even just focusing on the Estimator. When I look at
resource_manager_impl.cc
> it seems to me that you are getting rid of a very readable 30-lines loop, and
> replacing that with an invocation of a file which has 500 lines of templates
and
> has references to internals of STL implementation.
> How many other cases do we have where this will be used roughly?

1. ScopedMemoryUsage is in base/test, and is only used for testing. Sorry,
forgot to mention that. I'll also use it for testing sync MDP stuff.

2. It depends on how you look at it. std::map<> implementation is surely
complicated, but on the surface it exposes nice and clean API. Same thing with
EstimateMemoryUsage() - it's somewhat complicated (although most of it just
boilerplate), but it exposes simple API (just one function!). So three loops are
replaced with just three calls. In sync there are more, both EntryKernel and
Directory have 18 EstimateMemoryUsage() calls (and there are three more classes
not yet covered).

As we write more MDPs and go deeper in the code we'll need some established API
both for getting memory usage and for signalling that a class can estimate it's
own memory usage. EstimateMemoryUsage() solves both those goals:

1. You declare EstimateMemoryUsage() method in a class (or as a standalone
function) and that's a signal that class can report its memory usage.

2. To estimate memory usage, you just call EstimateMemoryUsage() on all class
variables. You don't need to care about what those variables are made of, and
don't need to maintain anything in case variable's type changes.

[Dmitry Skiba, 2016-10-25 04:55:06]

Another example: MDP for TabRestorer (http://crbug.com/645123):
https://codereview.chromium.org/2451583002

Clean and readable code (see for example
components/sessions/core/serialized_navigation_entry.cc).

I think EstimateMemoryUsage() nicely compliments MemoryDumpProvider, and allows
to quickly instrument parts of code to understand who allocates memory.

[Dmitry Skiba, 2016-10-26 22:27:29]

The CQ bit was checked by dskiba@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-26 22:27:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-26 22:38:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-26 22:38:55]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[Dmitry Skiba, 2016-10-26 23:47:45]

The CQ bit was checked by dskiba@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-26 23:48:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-27 00:35:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-27 00:35:28]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[DmitrySkiba, 2016-10-27 08:13:53]

dskiba@chromium.org changed reviewers:
+ dskiba@chromium.org

[DmitrySkiba, 2016-10-27 08:13:53]

Re: platform dependency.

The idea is to first account for platform differences so that results are
accurate, and then introduce accuracy margin so that future platform updates
don't break tests.

Last patchset introduces 10% margin, which should be enough.

[DmitrySkiba, 2016-10-28 00:05:45]

So, what do you think?

I have two new thoughts:

1. Since we now have estimation accuracy, maybe we don't need to do crazy and
account for platform differences - maybe modeling containers after libc++ will
be enough to fit into the accuracy margin?

2. Maybe rename to EstimateHeapMemoryUsage() to stress what 'memory' we're
talking about?

[Primiano Tucci (use gerrit), 2016-10-28 10:23:24]

On 2016/10/28 00:05:45, DmitrySkiba wrote:
> So, what do you think?
> 
> I have two new thoughts:
> 
> 1. Since we now have estimation accuracy, maybe we don't need to do crazy and
> account for platform differences - maybe modeling containers after libc++ will
> be enough to fit into the accuracy margin?
> 
> 2. Maybe rename to EstimateHeapMemoryUsage() to stress what 'memory' we're
> talking about?

So I have one problem here: the test code is way more complex than the actual
thing being tested, which looks a red herirng to me.
what is the thing you really want to test here? It feels to me you are testing a
mixture of your template math (good) AND the libc++ implementation (bad).
Also that ScopedMemoryUsage is introducing a pretty high level of complexity to
unittests, higher than your original class.
You shouldn't test the libc++ implementation, at least not in a unittest.
if you really are concerned about new versions of libc++ being bigger that
should be tracked as a perftest on chromeperf, not a unittest. But when, we
already have that coverage:
if somebody rolls a new version of libc++ which is dumber memory-wise, our
memory metrics for the existing benchmarks will go up and we'll spot it there.
And if they won't go up it will mean that the increase was not significant for
the usage we make in chrome.

I think the way you want your test work here is: create some of those map /
vectors etc and test that the estimated memory usage of them is consistent with
an estimated expectation you compute offline.
That will prevent that people refactoring that code will make unintended
functional changes, but without adding dependencies on the libc++  impl.

[DmitrySkiba, 2016-10-28 17:32:53]

On 2016/10/28 10:23:24, Primiano Tucci wrote:
> On 2016/10/28 00:05:45, DmitrySkiba wrote:
> > So, what do you think?
> > 
> > I have two new thoughts:
> > 
> > 1. Since we now have estimation accuracy, maybe we don't need to do crazy
and
> > account for platform differences - maybe modeling containers after libc++
will
> > be enough to fit into the accuracy margin?
> > 
> > 2. Maybe rename to EstimateHeapMemoryUsage() to stress what 'memory' we're
> > talking about?
> 
> So I have one problem here: the test code is way more complex than the actual
> thing being tested, which looks a red herirng to me.
> what is the thing you really want to test here? It feels to me you are testing
a
> mixture of your template math (good) AND the libc++ implementation (bad).

So the API is a function that returns memory estimate for various STL types.
Estimation is said to be within X% of the real usage. Tests are simply testing
that for all supported STL types.

From the implementation standpoint, to estimate memory usage of a container (say
std::map) we need to model its internals. I chose to model after libc++, which
is what is used on Android (and macOS, and Linux in the future). Initially I
wanted to be more precise on other platforms too, but now I see that we don't
need that since any reasonable implementation should be similar to libc++. So
I'm going to remove all platform-specific code and just use libc++ model.  Note
that modeling after libc++ doesn't bind us to libc++ in any way, we don't
include hidden headers, don't access libc++-specific types, etc.


> Also that ScopedMemoryUsage is introducing a pretty high level of complexity
to
> unittests, higher than your original class.

Actually, the tests can be written without ScopedMemoryUsage - we can use custom
allocator to count bytes. I'll have to remove some tests that use 'new', but
most will remain the same. I added ScopedMemoryUsage because it's more general,
and because I also use it for (temporary) tests for sync MDP.


> You shouldn't test the libc++ implementation, at least not in a unittest.
> if you really are concerned about new versions of libc++ being bigger that
> should be tracked as a perftest on chromeperf, not a unittest. But when, we
> already have that coverage:
> if somebody rolls a new version of libc++ which is dumber memory-wise, our
> memory metrics for the existing benchmarks will go up and we'll spot it there.
> And if they won't go up it will mean that the increase was not significant for
> the usage we make in chrome.

Isn't this what unittests are for? You have a functionality, and a test. If
environment changes so that assumptions that you baked in are no longer true,
the test fails, letting you know that you need to fix the implementation.


> I think the way you want your test work here is: create some of those map /
> vectors etc and test that the estimated memory usage of them is consistent
with
> an estimated expectation you compute offline.

So the unittests won't call EstimateMemoryUsage() at all?


> That will prevent that people refactoring that code will make unintended
> functional changes, but without adding dependencies on the libc++  impl.

There is no dependency on libc++ implementation. I have a model (of say
std::map) that is based on libc++, but the same model (will) also be used on
other platforms too (and even on future libc++ versions). The model is pretty
reasonable, and should always be valid. In fact you can check that for example
std::map (EstimateTreeMemoryUsage) has very similar implementations on all
platforms, it just differs by the order of fields.


Overall I agree that amount of testing code is several times more than the
implementation. Although I don't see that as a problem, I can think of ways to
reduce that based on what I said earlier:

1. I can kill ScopedMemoryUsage and test against custom allocator (i.e. instead
of std::map<T> I will test std::map<T..., test_allocator<T>>. This will also get
rid of some tests.

2. We can further cut down number of tests, for example I can remove String16
test, some/all array tests, unique_ptr tests, etc.


However, I don't want us to rely on metrics to spot regressions in this code. I
want to be able to trust numbers we'll get from slow reports etc., and for that
I need to be sure that EstimateMemoryUsage() functions are always within X%
margin of real usage. Unittest seems like a perfect way to assert that.


So I'm going to remove platform specific code from EstimateTreeMemoryUsage /
EstimateHashMapMemoryUsage and will wait for you response before doing anything
to tests.

[DmitrySkiba, 2016-11-01 05:20:57]

Sorry for delay, I'm stability sheriff this week, hopefully I'll get to this
tomorrow.

BTW, what do you think of my proposal to cut down number of tests?

[Primiano Tucci (use gerrit), 2016-11-01 19:04:04]

On 2016/10/28 17:32:53, DmitrySkiba wrote:
> On 2016/10/28 10:23:24, Primiano Tucci wrote:
> > On 2016/10/28 00:05:45, DmitrySkiba wrote:
> > > So, what do you think?
> > > 
> > > I have two new thoughts:
> > > 
> > > 1. Since we now have estimation accuracy, maybe we don't need to do crazy
> and
> > > account for platform differences - maybe modeling containers after libc++
> will
> > > be enough to fit into the accuracy margin?
> > > 
> > > 2. Maybe rename to EstimateHeapMemoryUsage() to stress what 'memory' we're
> > > talking about?
> > 
> > So I have one problem here: the test code is way more complex than the
actual
> > thing being tested, which looks a red herirng to me.
> > what is the thing you really want to test here? It feels to me you are
testing
> a
> > mixture of your template math (good) AND the libc++ implementation (bad).
> 
> So the API is a function that returns memory estimate for various STL types.
> Estimation is said to be within X% of the real usage. Tests are simply testing
> that for all supported STL types.
> 
> From the implementation standpoint, to estimate memory usage of a container
(say
> std::map) we need to model its internals. I chose to model after libc++, which
> is what is used on Android (and macOS, and Linux in the future). Initially I
> wanted to be more precise on other platforms too, but now I see that we don't
> need that since any reasonable implementation should be similar to libc++. So
> I'm going to remove all platform-specific code and just use libc++ model. 
Note
> that modeling after libc++ doesn't bind us to libc++ in any way, we don't
> include hidden headers, don't access libc++-specific types, etc.

Yup I get this, all this is fine.
The problem is when you have a test which sets expectations based on the current
behavior of libc and the test (but not the code under test) breaks if somebody
switches STL implementation.
In other words. If somebody rolls to libc++on-stereoids what will happen is:
 - your estimate_memory_usage.h code will still work but give some incorrect
results --> this part is good. 
 - the test will fail because the estimation above will not match the real
measured value --> this part is bad.

> > Also that ScopedMemoryUsage is introducing a pretty high level of complexity
> to
> > unittests, higher than your original class.
> 
> Actually, the tests can be written without ScopedMemoryUsage - we can use
custom
> allocator to count bytes. I'll have to remove some tests that use 'new', but
> most will remain the same. I added ScopedMemoryUsage because it's more
general,
> and because I also use it for (temporary) tests for sync MDP.

What I don't get is: why you can have a test with no SMU and no custom
allocator, where you just:
- Fill a map/vector/whatever
- DCHECK_EQ(42, EstimateMemoryUsage(my_map)
Where 42 is a value that you derive from the current implementation.
This test will not break in the scenario above. Will only break if somebody
changes the estimation code without updating the test.

> > You shouldn't test the libc++ implementation, at least not in a unittest.
> > if you really are concerned about new versions of libc++ being bigger that
> > should be tracked as a perftest on chromeperf, not a unittest. But when, we
> > already have that coverage:
> > if somebody rolls a new version of libc++ which is dumber memory-wise, our
> > memory metrics for the existing benchmarks will go up and we'll spot it
there.
> > And if they won't go up it will mean that the increase was not significant
for
> > the usage we make in chrome.
> 
> Isn't this what unittests are for? You have a functionality, and a test. If
> environment changes so that assumptions that you baked in are no longer true,
> the test fails, letting you know that you need to fix the implementation.

The only problem here is that if the environment (libc++) changes and the test
breaks, the problem in this case is still in the functionality (the estimation)
not in the environment.
It is not fare to cause problems to people that try to roll libc++ just because
the future versions of libc++ won't match your mental problem  
The other problem is that generally the environment is chrome code, so a failing
test can be helpful. but not in this case. There is nothing chromium devs can do
if the new version of libc++ breaks your model.

> 
> 
> > I think the way you want your test work here is: create some of those map /
> > vectors etc and test that the estimated memory usage of them is consistent
> with
> > an estimated expectation you compute offline.
> 
> So the unittests won't call EstimateMemoryUsage() at all?

Of course the have to, otherwise what do you test?
My proposal is the other way round. They should not measure the memory really
used by std::map & co in any way.


> > That will prevent that people refactoring that code will make unintended
> > functional changes, but without adding dependencies on the libc++  impl.
> 
> There is no dependency on libc++ implementation. I have a model (of say
> std::map) that is based on libc++, but the same model (will) also be used on
> other platforms too (and even on future libc++ versions). The model is pretty
> reasonable, and should always be valid. In fact you can check that for example
> std::map (EstimateTreeMemoryUsage) has very similar implementations on all
> platforms, it just differs by the order of fields.

If libc++ implementation changes your test breaks. This is a dependency.

> Overall I agree that amount of testing code is several times more than the
> implementation. Although I don't see that as a problem, I can think of ways to
> reduce that based on what I said earlier:
> 
> 1. I can kill ScopedMemoryUsage and test against custom allocator (i.e.
instead
> of std::map<T> I will test std::map<T..., test_allocator<T>>. This will also
get
> rid of some tests.

See above. custom allocator doesn't solve the root problem.

> 
> 2. We can further cut down number of tests, for example I can remove String16
> test, some/all array tests, unique_ptr tests, etc.
> 
> 
> However, I don't want us to rely on metrics to spot regressions in this code.
I
> want to be able to trust numbers we'll get from slow reports etc., and for
that
> I need to be sure that EstimateMemoryUsage() functions are always within X%
> margin of real usage. Unittest seems like a perfect way to assert that.

Here is the problem: the way this unittest is designed pushes this burden on
other people who roll NDK or compiler toolchain not you.
This is not fair. You cannot block other people because you want to estimate STL
memory. 
What do you expect somebody like jbudorick@ or thakis@ or agrieve@ (looking at
the list of people whoe recently rolled toolchain/ndk) to do the day the roll
libc++ and this test fails?

 
> So I'm going to remove platform specific code from EstimateTreeMemoryUsage /
> EstimateHashMapMemoryUsage and will wait for you response before doing
anything
> to tests.

[DmitrySkiba, 2016-11-03 16:25:22]

I see, I think I get your points now:

1. It's the test that adds dependency to the libc++, and that can be problematic
for people who roll new NDKs.

2. So instead of testing against current libc++ we should just test EMU
functionally, i.e. EXPECT_EQ(42, EstimateMemoryUsage(map)).

3. That way even if libc++ implementation changes tests won't break.


I feel like in this setup it doesn't make sense to test EMU functions at all.
I.e. the fact that EstimateMemoryUsage(map) returns 42 is only valuable if 42 is
close enough (currently 10%) to the real memory usage of |map|. If we remove
connection to the "real usage", which is now established by tests, then there is
very little point to have tests at all.

I.e. those two cases are similarly bad from a memory usage estimation
standpoint:

1. Real libc++ usage is 42, EstimateMemoryUsage(map) changed to return 50.

2. Real libc++ usage changed to 50, EstimateMemoryUsage(map) still returns 42.

In your proposal tests would only catch #1, but not #2.

I think it's important for us to trust EMU numbers (case #2). In the end those
numbers will get into slow reports and we'll use them to make points to other
teams. We should be able to say something like "those numbers are X% accurate",
which we won't be able to say without tests.


There are also two important points about tests to keep in mind:

1. Not only libc++ is tested. There are several models for different platforms
(although I'm thinking of just using libc++ model), and for example tests will
break if MSVC drastically changes its implementation. I.e. right now tests
depend on many platforms, now just libc++.

2. The checks are not exact. Currently tests pass if estimated usage lies within
10% of real usage. This should be enough for any reasonable change, although
it's definitely possible to bloat an implementation so that it starts consuming
10% more.


Having said that, do you still think we should cut platform dependency in tests?
I'm not strongly opposed to that, just wanted to give my view before doing
change.

[DmitrySkiba, 2016-11-08 05:37:42]

Tests simplified, PTAL.

Any idea on how to handle 64/32-bit differences?

I don't want to do something like

#if defined(__LP64__)
EXPECT_EQ(42u, EstimateMemoryUsage(map));
#else
EXPECT_EQ(20u, EstimateMemoryUsage(map));
#endif

everywhere.

[Primiano Tucci (use gerrit), 2016-11-08 14:09:15]

primiano@chromium.org changed reviewers:
+ dcheng@chromium.org

[Primiano Tucci (use gerrit), 2016-11-08 14:09:16]

Ok generally the idea and the new test strategy LGTM.
I like the final composability effect.
I would just ask if you could get an eye also from some C++11 expert given the
amount of template magic in here.
My major doubt is why you need the "using
base::trace_event::EstimateMemoryUsage"? Is it for accessing the class-defined
EMU? Shouldn't you use a traits pattern in this case?

I honestly am not sure about what is the best way to do this, so would feel more
comfortable if all this magic was checked by somebody who is more familiar than
me on this.

In this regard, let me see if I can spot a +dcheng@ in the wild ;-)

https://codereview.chromium.org/2440393002/diff/120001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/2440393002/diff/120001/base/BUILD.gn#newcode902
base/BUILD.gn:902: "trace_event/estimate_memory_usage.h",
Generally all the source file names I see are nouns.
Maybe memory_usage_estimator.h?

https://codereview.chromium.org/2440393002/diff/120001/base/trace_event/estim...
File base/trace_event/estimate_memory_usage.h (right):

https://codereview.chromium.org/2440393002/diff/120001/base/trace_event/estim...
base/trace_event/estimate_memory_usage.h:53: // 1. It starts with 'using'
declaration. This makes everything defined in
?? Why this doesn't work if you just say base::trace_event::EstimateMemoryUsage?
You are not importing a namespace

https://codereview.chromium.org/2440393002/diff/120001/base/trace_event/estim...
base/trace_event/estimate_memory_usage.h:71: auto EstimateMemoryUsage(const T&
object)
out of curiosity isn't this always size_t? Why do you need a trailing return
type here?
Or is it forced because of the lambda?

https://codereview.chromium.org/2440393002/diff/120001/base/trace_event/estim...
File base/trace_event/estimate_memory_usage_unittest.cc (right):

https://codereview.chromium.org/2440393002/diff/120001/base/trace_event/estim...
base/trace_event/estimate_memory_usage_unittest.cc:100: EXPECT_EQ(8u,
EstimateMemoryUsage(ptr));
s/8u/sizeof(void*)/ ?

https://codereview.chromium.org/2440393002/diff/120001/base/trace_event/estim...
base/trace_event/estimate_memory_usage_unittest.cc:106: int payload[10];
s/int/uint32_t/ to keep it a bit more readable

[Primiano Tucci (use gerrit), 2016-11-08 14:09:59]

On 2016/11/08 05:37:42, DmitrySkiba wrote:
> Tests simplified, PTAL.
> 
> Any idea on how to handle 64/32-bit differences?
> 
> I don't want to do something like
> 
> #if defined(__LP64__)
> EXPECT_EQ(42u, EstimateMemoryUsage(map));
> #else
> EXPECT_EQ(20u, EstimateMemoryUsage(map));
> #endif
> 
> everywhere.

Express them in terms of sizeof(void*), e.g. 1000 * sizeof(void*) ?

[DmitrySkiba, 2016-11-08 17:47:02]

On 2016/11/08 14:09:59, Primiano Tucci - mostly ooo wrote:
> On 2016/11/08 05:37:42, DmitrySkiba wrote:
> > Tests simplified, PTAL.
> > 
> > Any idea on how to handle 64/32-bit differences?
> > 
> > I don't want to do something like
> > 
> > #if defined(__LP64__)
> > EXPECT_EQ(42u, EstimateMemoryUsage(map));
> > #else
> > EXPECT_EQ(20u, EstimateMemoryUsage(map));
> > #endif
> > 
> > everywhere.
> 
> Express them in terms of sizeof(void*), e.g. 1000 * sizeof(void*) ?

Not generally possible. Node struct for tree has several of them, for example.
I'm thinking of macro that takes two values and chooses one depending on
bitness: EXPECT_USAGE(20, 42, EstimateMemoryUsage(map)).

[DmitrySkiba, 2016-11-08 17:47:14]

https://codereview.chromium.org/2440393002/diff/120001/base/trace_event/estim...
File base/trace_event/estimate_memory_usage.h (right):

https://codereview.chromium.org/2440393002/diff/120001/base/trace_event/estim...
base/trace_event/estimate_memory_usage.h:53: // 1. It starts with 'using'
declaration. This makes everything defined in
On 2016/11/08 14:09:16, Primiano Tucci - mostly ooo wrote:
> ?? Why this doesn't work if you just say
base::trace_event::EstimateMemoryUsage?
> You are not importing a namespace 

It won't work if EstimateMemoryUsage() for T is defined as a free function in
T's namespace. In that case it should be just EstimateMemoryUsage() so that ADL
finds correct one.

And I really don't want callers to think about where and how EMU is defined. You
do using, and then just call EstimateMemoryUsage() on all eligible members.

https://codereview.chromium.org/2440393002/diff/120001/base/trace_event/estim...
base/trace_event/estimate_memory_usage.h:71: auto EstimateMemoryUsage(const T&
object)
On 2016/11/08 14:09:16, Primiano Tucci - mostly ooo wrote:
> out of curiosity isn't this always size_t? Why do you need a trailing return
> type here?
> Or is it forced because of the lambda?

The trick here is that if T doesn't have EMU member function, this global
function is not considered. I.e. decltype(object.EstimateMemoryUsage()) silently
fails and removes this function from candidate list for T. Function can be
written without trailing return, with std::declval, but it'll be uglier.

[DmitrySkiba, 2016-11-08 18:41:47]

dskiba@chromium.org changed reviewers:
+ dtrainor@chromium.org, ericrk@chromium.org

[DmitrySkiba, 2016-11-08 18:41:51]

dtrainor@, ericrk@: guys, please review changes in cc/ and ui/. The changes
rename some functions that ssid@ introduced in
https://codereview.chromium.org/2406103002

[dcheng, 2016-11-08 19:31:48]

Some random thoughts WRT templates.

https://codereview.chromium.org/2440393002/diff/120001/base/trace_event/estim...
File base/trace_event/estimate_memory_usage.h (right):

https://codereview.chromium.org/2440393002/diff/120001/base/trace_event/estim...
base/trace_event/estimate_memory_usage.h:53: // 1. It starts with 'using'
declaration. This makes everything defined in
On 2016/11/08 17:47:14, DmitrySkiba wrote:
> On 2016/11/08 14:09:16, Primiano Tucci - mostly ooo wrote:
> > ?? Why this doesn't work if you just say
> base::trace_event::EstimateMemoryUsage?
> > You are not importing a namespace 
> 
> It won't work if EstimateMemoryUsage() for T is defined as a free function in
> T's namespace. In that case it should be just EstimateMemoryUsage() so that
ADL
> finds correct one.
> 
> And I really don't want callers to think about where and how EMU is defined.
You
> do using, and then just call EstimateMemoryUsage() on all eligible members.

FWIW, if we can figure out some way to avoid 'using', I think that'd be better.
std::swap is supposed to be used the same way, but the 'using' statement is
often just omitted completely.

(ParamTraits for IPC get around this by requiring everyone to specialize
ParamTraits in the IPC namespace. Maybe we could just add a namespace
requirement on the traits class here; that doesn't seem unreasonable to me,
since the EMU function itself is in base...)

https://codereview.chromium.org/2440393002/diff/120001/base/trace_event/estim...
base/trace_event/estimate_memory_usage.h:93: size_t EstimateMemoryUsage(const
std::unique_ptr<T>& ptr);
Note that unique_ptr has a custom delete parameter.

https://codereview.chromium.org/2440393002/diff/120001/base/trace_event/estim...
base/trace_event/estimate_memory_usage.h:177: // to force compilers to include
type of T in the error message.
clang and msvc will both tell you which template instantiation caused this to
fail, which includes all the template params.

However, we can't just static_assert(false, ...) here: it's important to make
sure the static assert is dependent on a template type:
http://stackoverflow.com/questions/14637356/static-assert-fails-compilation-e...

I'd document this latter reason probably, since it's somewhat subtle.

[DmitrySkiba, 2016-11-08 22:14:21]

On 2016/11/08 19:31:48, dcheng wrote:
> Some random thoughts WRT templates.
> 
>
https://codereview.chromium.org/2440393002/diff/120001/base/trace_event/estim...
> File base/trace_event/estimate_memory_usage.h (right):
> 
>
https://codereview.chromium.org/2440393002/diff/120001/base/trace_event/estim...
> base/trace_event/estimate_memory_usage.h:53: // 1. It starts with 'using'
> declaration. This makes everything defined in
> On 2016/11/08 17:47:14, DmitrySkiba wrote:
> > On 2016/11/08 14:09:16, Primiano Tucci - mostly ooo wrote:
> > > ?? Why this doesn't work if you just say
> > base::trace_event::EstimateMemoryUsage?
> > > You are not importing a namespace 
> > 
> > It won't work if EstimateMemoryUsage() for T is defined as a free function
in
> > T's namespace. In that case it should be just EstimateMemoryUsage() so that
> ADL
> > finds correct one.
> > 
> > And I really don't want callers to think about where and how EMU is defined.
> You
> > do using, and then just call EstimateMemoryUsage() on all eligible members.
> 
> FWIW, if we can figure out some way to avoid 'using', I think that'd be
better.
> std::swap is supposed to be used the same way, but the 'using' statement is
> often just omitted completely.
> 
> (ParamTraits for IPC get around this by requiring everyone to specialize
> ParamTraits in the IPC namespace. Maybe we could just add a namespace
> requirement on the traits class here; that doesn't seem unreasonable to me,
> since the EMU function itself is in base...)

It might be possible to get rid of it (and just always define it in
base::trace_event), but why? What is the benefit of repeated base::trace_event::
prefix? Is it something prescribed by our code style?

> 
>
https://codereview.chromium.org/2440393002/diff/120001/base/trace_event/estim...
> base/trace_event/estimate_memory_usage.h:93: size_t EstimateMemoryUsage(const
> std::unique_ptr<T>& ptr);
> Note that unique_ptr has a custom delete parameter.
> 
>
https://codereview.chromium.org/2440393002/diff/120001/base/trace_event/estim...
> base/trace_event/estimate_memory_usage.h:177: // to force compilers to include
> type of T in the error message.
> clang and msvc will both tell you which template instantiation caused this to
> fail, which includes all the template params.
> 
> However, we can't just static_assert(false, ...) here: it's important to make
> sure the static assert is dependent on a template type:
>
http://stackoverflow.com/questions/14637356/static-assert-fails-compilation-e...
> 
> I'd document this latter reason probably, since it's somewhat subtle.

Thanks for the link. I'll update the text (and will also use false_type instead
of X).

[dcheng, 2016-11-09 00:35:51]

On 2016/11/08 22:14:21, DmitrySkiba wrote:
> On 2016/11/08 19:31:48, dcheng wrote:
> > Some random thoughts WRT templates.
> > 
> >
>
https://codereview.chromium.org/2440393002/diff/120001/base/trace_event/estim...
> > File base/trace_event/estimate_memory_usage.h (right):
> > 
> >
>
https://codereview.chromium.org/2440393002/diff/120001/base/trace_event/estim...
> > base/trace_event/estimate_memory_usage.h:53: // 1. It starts with 'using'
> > declaration. This makes everything defined in
> > On 2016/11/08 17:47:14, DmitrySkiba wrote:
> > > On 2016/11/08 14:09:16, Primiano Tucci - mostly ooo wrote:
> > > > ?? Why this doesn't work if you just say
> > > base::trace_event::EstimateMemoryUsage?
> > > > You are not importing a namespace 
> > > 
> > > It won't work if EstimateMemoryUsage() for T is defined as a free function
> in
> > > T's namespace. In that case it should be just EstimateMemoryUsage() so
that
> > ADL
> > > finds correct one.
> > > 
> > > And I really don't want callers to think about where and how EMU is
defined.
> > You
> > > do using, and then just call EstimateMemoryUsage() on all eligible
members.
> > 
> > FWIW, if we can figure out some way to avoid 'using', I think that'd be
> better.
> > std::swap is supposed to be used the same way, but the 'using' statement is
> > often just omitted completely.
> > 
> > (ParamTraits for IPC get around this by requiring everyone to specialize
> > ParamTraits in the IPC namespace. Maybe we could just add a namespace
> > requirement on the traits class here; that doesn't seem unreasonable to me,
> > since the EMU function itself is in base...)
> 
> It might be possible to get rid of it (and just always define it in
> base::trace_event), but why? What is the benefit of repeated
base::trace_event::
> prefix? Is it something prescribed by our code style?

The reasoning for this is simply that most code *will* forget to 'using
base::trace_event::EstimateMemoryUsage' before calling it (see std::swap for
example). This is simply not a common idiom, and it's easy to forget. Often
times, it won't matter, but sometimes it will (and lead to compile errors and
incorrect workarounds).

> 
> > 
> >
>
https://codereview.chromium.org/2440393002/diff/120001/base/trace_event/estim...
> > base/trace_event/estimate_memory_usage.h:93: size_t
EstimateMemoryUsage(const
> > std::unique_ptr<T>& ptr);
> > Note that unique_ptr has a custom delete parameter.
> > 
> >
>
https://codereview.chromium.org/2440393002/diff/120001/base/trace_event/estim...
> > base/trace_event/estimate_memory_usage.h:177: // to force compilers to
include
> > type of T in the error message.
> > clang and msvc will both tell you which template instantiation caused this
to
> > fail, which includes all the template params.
> > 
> > However, we can't just static_assert(false, ...) here: it's important to
make
> > sure the static assert is dependent on a template type:
> >
>
http://stackoverflow.com/questions/14637356/static-assert-fails-compilation-e...
> > 
> > I'd document this latter reason probably, since it's somewhat subtle.
> 
> Thanks for the link. I'll update the text (and will also use false_type
instead
> of X).

[DmitrySkiba, 2016-11-09 01:14:06]

On 2016/11/09 00:35:51, dcheng wrote:
...
> > It might be possible to get rid of it (and just always define it in
> > base::trace_event), but why? What is the benefit of repeated
> base::trace_event::
> > prefix? Is it something prescribed by our code style?
> 
> The reasoning for this is simply that most code *will* forget to 'using
> base::trace_event::EstimateMemoryUsage' before calling it (see std::swap for
> example). This is simply not a common idiom, and it's easy to forget. Often
> times, it won't matter, but sometimes it will (and lead to compile errors and
> incorrect workarounds).

OK, I this makes sense, so I'll change the comment to suggest putting EMUs in
base::trace_event and remove using.

[DmitrySkiba, 2016-11-09 06:10:11]

PTAL again.

Updated comment to recommend putting EMUs in base::trace_event, and fixed tests
on 32/64 bit platforms. Also fixed issue with unordered containers which
depended on bucket_count, which can vary across platforms.

[Dmitry Skiba, 2016-11-09 18:46:13]

The CQ bit was checked by dskiba@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-09 18:46:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-09 20:18:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-09 20:18:05]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Dmitry Skiba, 2016-11-09 20:43:01]

The CQ bit was checked by dskiba@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-09 20:43:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-09 22:09:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-09 22:09:10]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-11-10 06:45:46]

LGTM with some nits.

My main concern here is around potential binary bloat, since all this stuff will
end up inlined. Are there plans to use this in lots of places throughout the
codebase, or will it be fairly targeted?

https://codereview.chromium.org/2440393002/diff/220001/base/trace_event/memor...
File base/trace_event/memory_usage_estimator.h (right):

https://codereview.chromium.org/2440393002/diff/220001/base/trace_event/memor...
base/trace_event/memory_usage_estimator.h:166: template <class T, class X =
void>
Nit: I think class X = void may no longer be necessary

https://codereview.chromium.org/2440393002/diff/220001/base/trace_event/memor...
base/trace_event/memory_usage_estimator.h:170: static_assert(std::is_same<T,
std::false_type>::value,
It's technically possible for std::false to be supplied as a type here, right?

I think I previously tried sizeof(T) == 0 as an always impossible assert, so
maybe that will work here.

https://codereview.chromium.org/2440393002/diff/220001/base/trace_event/memor...
base/trace_event/memory_usage_estimator.h:228: const char* cstr =
reinterpret_cast<const char*>(string.c_str());
Perhaps use uint8_t to make it clear that we explicitly want to work with byte
offsets (my first thought when I read this is "shouldn't this be using const
value_type* cstr = ...)

[Primiano Tucci (use gerrit), 2016-11-10 11:09:38]

Thanks a lot Daniel for the help here. Still LGTM from my side.

On 2016/11/10 06:45:46, dcheng wrote:
> My main concern here is around potential binary bloat, since all this stuff
will
> end up inlined. Are there plans to use this in lots of places throughout the
> codebase

So by looking at that header, it seems to me that most of that boilerplate ends
up being sums of trivial accessors and constexprs (mostly sizeof stuff).
The major exception is EstimateIterableMemoryUsage. I wonder if writing
EstimateIterableMemoryUsage in terms of a template function that takes a pointer
to the EMU function would make the resulting for-loop more
identical-code-folding friendly.
In other words my speculation here is that:

size_t InvokeEstimationFunction(size_t (fn*)(void*), void* iterable_entry); 
// defined in a .cc file  for the only sake of preventing the |fn| to be
expanded inline in the loop.
{
  return fn(iterable_entry);
}

template<T> size_t EstimateIterableMemoryUsage(const T& collection, size_t
(estimation_function*)(void*)) {
 size_t res = 0;
  for (const T& t : collection)
     size += InvokeEstimationFunction(estimation_function, &t)
  return size;
}

would be ICF better (on the other side, my latest speculations on compiler
optimizations brought me to misery :) ).
But at the same time this seems more complexity and less readability, so not
100% sure we should go there. WDYT?

>, or will it be fairly targeted?
I think this is for a bunch of classes like the ones in this CL (and another
upcoming one about sync) right now.

[Primiano Tucci (use gerrit), 2016-11-10 11:10:30]

On 2016/11/10 11:09:38, Primiano Tucci - mostly ooo wrote:
> Thanks a lot Daniel for the help here. Still LGTM from my side.

Clearly % Daniel's pending comments.

[DmitrySkiba, 2016-11-10 17:39:08]

On 2016/11/10 06:45:46, dcheng wrote:
> LGTM with some nits.
> 
> My main concern here is around potential binary bloat, since all this stuff
will
> end up inlined. Are there plans to use this in lots of places throughout the
> codebase, or will it be fairly targeted?

Do you think templates will inflate binary sizer more than manual looping? 

Right now it's just two places, this CL and and sync MDP
(https://codereview.chromium.org/2452713003). I don't think there will be many
more, maybe one or two more MDPs where we might need this.

Also, I think we can reduce bloat a bit by explicitly instantiating EMUs for
std::string and string16 in .cc file.

> 
>
https://codereview.chromium.org/2440393002/diff/220001/base/trace_event/memor...
> File base/trace_event/memory_usage_estimator.h (right):
> 
>
https://codereview.chromium.org/2440393002/diff/220001/base/trace_event/memor...
> base/trace_event/memory_usage_estimator.h:166: template <class T, class X =
> void>
> Nit: I think class X = void may no longer be necessary

Hmm, but I use the second argument to enable_if specializations - is there
another way to do it?

> 
>
https://codereview.chromium.org/2440393002/diff/220001/base/trace_event/memor...
> base/trace_event/memory_usage_estimator.h:170: static_assert(std::is_same<T,
> std::false_type>::value,
> It's technically possible for std::false to be supplied as a type here, right?
> 
> I think I previously tried sizeof(T) == 0 as an always impossible assert, so
> maybe that will work here.

The idea I got from SO question you linked earlier is that since sizeof(T) is
never 0, there is nothing stopping compiler from treating 'sizeof(T) == 0' as a
constant. Also, it's highly unlikely for std::false_type to be used as T here,
but even if it is used, it will go to 'is_trivially_destructible'
specialization.

> 
>
https://codereview.chromium.org/2440393002/diff/220001/base/trace_event/memor...
> base/trace_event/memory_usage_estimator.h:228: const char* cstr =
> reinterpret_cast<const char*>(string.c_str());
> Perhaps use uint8_t to make it clear that we explicitly want to work with byte
> offsets (my first thought when I read this is "shouldn't this be using const
> value_type* cstr = ...)

Will do.

[DmitrySkiba, 2016-11-10 19:32:37]

Friendly ping


dtrainor@, ericrk@: guys, please review changes in cc/ and ui/.

The changes rename some functions that ssid@ introduced in
https://codereview.chromium.org/2406103002

[David Trainor- moved to gerrit, 2016-11-10 22:29:50]

ui/ lgtm!  Thanks!

[ericrk, 2016-11-10 23:49:53]

cc/ LGTM

[dcheng, 2016-11-11 00:01:42]

On 2016/11/10 17:39:08, DmitrySkiba wrote:
> On 2016/11/10 06:45:46, dcheng wrote:
> > LGTM with some nits.
> > 
> > My main concern here is around potential binary bloat, since all this stuff
> will
> > end up inlined. Are there plans to use this in lots of places throughout the
> > codebase, or will it be fairly targeted?
> 
> Do you think templates will inflate binary sizer more than manual looping? 
> 
> Right now it's just two places, this CL and and sync MDP
> (https://codereview.chromium.org/2452713003). I don't think there will be many
> more, maybe one or two more MDPs where we might need this.
> 
> Also, I think we can reduce bloat a bit by explicitly instantiating EMUs for
> std::string and string16 in .cc file.

The main thing I'd be worried about is the iterable ones getting unrolled and
inlined: we had this issue in some v8 object logging code, and it blew up the
binary size considerably (IIRC, the logging code ended up quadrupling in size,
or more).

> 
> > 
> >
>
https://codereview.chromium.org/2440393002/diff/220001/base/trace_event/memor...
> > File base/trace_event/memory_usage_estimator.h (right):
> > 
> >
>
https://codereview.chromium.org/2440393002/diff/220001/base/trace_event/memor...
> > base/trace_event/memory_usage_estimator.h:166: template <class T, class X =
> > void>
> > Nit: I think class X = void may no longer be necessary
> 
> Hmm, but I use the second argument to enable_if specializations - is there
> another way to do it?

You're right, I missed this.

> 
> > 
> >
>
https://codereview.chromium.org/2440393002/diff/220001/base/trace_event/memor...
> > base/trace_event/memory_usage_estimator.h:170: static_assert(std::is_same<T,
> > std::false_type>::value,
> > It's technically possible for std::false to be supplied as a type here,
right?
> > 
> > I think I previously tried sizeof(T) == 0 as an always impossible assert, so
> > maybe that will work here.
> 
> The idea I got from SO question you linked earlier is that since sizeof(T) is
> never 0, there is nothing stopping compiler from treating 'sizeof(T) == 0' as
a
> constant. Also, it's highly unlikely for std::false_type to be used as T here,
> but even if it is used, it will go to 'is_trivially_destructible'
> specialization.

OK. I don't feel strongly about it (I don't think any compiler is currently
smart enough to treat sizeof(T) == 0 as always false though.

> 
> > 
> >
>
https://codereview.chromium.org/2440393002/diff/220001/base/trace_event/memor...
> > base/trace_event/memory_usage_estimator.h:228: const char* cstr =
> > reinterpret_cast<const char*>(string.c_str());
> > Perhaps use uint8_t to make it clear that we explicitly want to work with
byte
> > offsets (my first thought when I read this is "shouldn't this be using const
> > value_type* cstr = ...)
> 
> Will do.

[DmitrySkiba, 2016-11-11 01:14:01]

On 2016/11/11 00:01:42, dcheng wrote:
> ... 
> The main thing I'd be worried about is the iterable ones getting unrolled and
> inlined: we had this issue in some v8 object logging code, and it blew up the
> binary size considerably (IIRC, the logging code ended up quadrupling in size,
> or more).

But here we have different types at each instantiation, and we only instantiate
it once, since the pattern is to call EMUs on fields only inside type's EMU
implementation. So it looks to me that (usually) there won't be any bloat, only
the necessary code to iterate containers of different types.

[Primiano Tucci (use gerrit), 2016-11-11 10:17:03]

On 2016/11/11 01:14:01, DmitrySkiba wrote:
> On 2016/11/11 00:01:42, dcheng wrote:
> > ... 
> > The main thing I'd be worried about is the iterable ones getting unrolled
and
> > inlined: we had this issue in some v8 object logging code, and it blew up
the
> > binary size considerably (IIRC, the logging code ended up quadrupling in
size,
> > or more).
> 
> But here we have different types at each instantiation, and we only
instantiate
> it once, since the pattern is to call EMUs on fields only inside type's EMU
> implementation. So it looks to me that (usually) there won't be any bloat,
only
> the necessary code to iterate containers of different types.

Well my point here was precisely this: if you prevent the EMU call to be
unrolled in the loop and instead pass a fptr (or whatever similar), the
resulting N template instantiation will very likely be identically-code-folded.
There should be no difference in iterating over a vector<Foo> vs vector<Bar> and
calling a fptr on their element. Instead if you unroll the specialized call, now
you end up with one loop for Foo and one for Bar.

[DmitrySkiba, 2016-11-11 16:19:39]

On 2016/11/11 10:17:03, Primiano Tucci wrote:
> On 2016/11/11 01:14:01, DmitrySkiba wrote:
> > On 2016/11/11 00:01:42, dcheng wrote:
> > > ... 
> > > The main thing I'd be worried about is the iterable ones getting unrolled
> and
> > > inlined: we had this issue in some v8 object logging code, and it blew up
> the
> > > binary size considerably (IIRC, the logging code ended up quadrupling in
> size,
> > > or more).
> > 
> > But here we have different types at each instantiation, and we only
> instantiate
> > it once, since the pattern is to call EMUs on fields only inside type's EMU
> > implementation. So it looks to me that (usually) there won't be any bloat,
> only
> > the necessary code to iterate containers of different types.
> 
> Well my point here was precisely this: if you prevent the EMU call to be
> unrolled in the loop and instead pass a fptr (or whatever similar), the
> resulting N template instantiation will very likely be
identically-code-folded.
> There should be no difference in iterating over a vector<Foo> vs vector<Bar>
and
> calling a fptr on their element. Instead if you unroll the specialized call,
now
> you end up with one loop for Foo and one for Bar.

Hmm, OK, let's get back to this idea if there is a size regression. For now I'll
commit this to unblock sync MDP.

[DmitrySkiba, 2016-11-11 16:19:45]

The CQ bit was checked by dskiba@chromium.org

[DmitrySkiba, 2016-11-11 16:19:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2440393002/#ps240001
(title: "Fix strings EMU, instantiate common ones in .cc file")

[commit-bot: I haz the power, 2016-11-11 16:20:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-11 16:35:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-11 16:35:55]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[DmitrySkiba, 2016-11-11 17:02:35]

The CQ bit was checked by dskiba@chromium.org

[DmitrySkiba, 2016-11-11 17:02:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, dtrainor@chromium.org, dcheng@chromium.org,
ericrk@chromium.org
Link to the patchset: https://codereview.chromium.org/2440393002/#ps260001
(title: "Add BASE_EXPORT to fix component build")

[commit-bot: I haz the power, 2016-11-11 17:03:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-11 18:02:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-11 18:02:24]

Failed to apply patch for base/BUILD.gn:
While running git apply --index -p1;
  error: patch failed: base/BUILD.gn:1981
  error: base/BUILD.gn: patch does not apply

Patch:       base/BUILD.gn
Index: base/BUILD.gn
diff --git a/base/BUILD.gn b/base/BUILD.gn
index
a3e3205eae9f137c15c774fdff736a88124201f8..9c258ece438130dad7eef47fd84ec9aa77f37b7b
100644
--- a/base/BUILD.gn
+++ b/base/BUILD.gn
@@ -931,6 +931,8 @@ component("base") {
     "trace_event/memory_dump_session_state.h",
     "trace_event/memory_infra_background_whitelist.cc",
     "trace_event/memory_infra_background_whitelist.h",
+    "trace_event/memory_usage_estimator.cc",
+    "trace_event/memory_usage_estimator.h",
     "trace_event/process_memory_dump.cc",
     "trace_event/process_memory_dump.h",
     "trace_event/process_memory_maps.cc",
@@ -1981,6 +1983,7 @@ test("base_unittests") {
     "trace_event/java_heap_dump_provider_android_unittest.cc",
     "trace_event/memory_allocator_dump_unittest.cc",
     "trace_event/memory_dump_manager_unittest.cc",
+    "trace_event/memory_usage_estimator_unittest.cc",
     "trace_event/process_memory_dump_unittest.cc",
     "trace_event/trace_config_unittest.cc",
     "trace_event/trace_event_argument_unittest.cc",

[DmitrySkiba, 2016-11-11 19:02:52]

The CQ bit was checked by dskiba@chromium.org

[DmitrySkiba, 2016-11-11 19:02:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, dtrainor@chromium.org, dcheng@chromium.org,
ericrk@chromium.org
Link to the patchset: https://codereview.chromium.org/2440393002/#ps280001
(title: "Rebase")

[commit-bot: I haz the power, 2016-11-11 19:03:23]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-11 20:31:14]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2016-11-11 21:00:10]

Description was changed from

==========
[tracing] Implement composable memory usage estimators.

When writing MemoryDumpProviders it's sometimes necessary to know how
much memory a particular class is responsible for. The answer is not easy
when class' memory usage is spread across several members which use STL
containers.

This CL introduces EstimateMemoryUsage() family of methods to accurately
estimate memory usage of STL containers. I.e. if EstimateMemoryUsage() is
implemented for type T, the memory usage of std::map<int, std::vector<T>>
is just one EstimateMemoryUsage() call away (instead of two nested loops),
and also includes memory used by std::map/vector (which can be big!).

The change is needed for sync MDP provider, but can also be used in other
MDPs. As an example this CL converts "ui/resource_manager" MDP to use
estimators resulting in cleaner code.

BUG=649065
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
[tracing] Implement composable memory usage estimators.

When writing MemoryDumpProviders it's sometimes necessary to know how
much memory a particular class is responsible for. The answer is not easy
when class' memory usage is spread across several members which use STL
containers.

This CL introduces EstimateMemoryUsage() family of methods to accurately
estimate memory usage of STL containers. I.e. if EstimateMemoryUsage() is
implemented for type T, the memory usage of std::map<int, std::vector<T>>
is just one EstimateMemoryUsage() call away (instead of two nested loops),
and also includes memory used by std::map/vector (which can be big!).

The change is needed for sync MDP provider, but can also be used in other
MDPs. As an example this CL converts "ui/resource_manager" MDP to use
estimators resulting in cleaner code.

BUG=649065
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

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

[commit-bot: I haz the power, 2016-11-11 21:00:12]

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