Adjust heap profiler integration for ContiguousContainer.

Adjust heap profiler integration for ContiguousContainer.

Firstly, change the type passed to the allocator to always be BaseElementType.
Otherwise, the buffer will be ascribed to whatever derived type was being
allocated when the buffer overflowed.

Secondly, confine the typeName parameter to only those places where the type
is passed from the ContiguousContainer template, through the base class, to
the allocator. (There are two entry points: initial capacity and new allocation.)

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

[jbroman, 2016-01-20 21:28:09]

jbroman@chromium.org changed reviewers:
+ ruuda@google.com

[jbroman, 2016-01-20 21:28:10]

PTAL. I think this makes the integration a little less intrusive, and should
also make the resulting data clearer. For instance, it seems a little more
honest to attribute the DisplayItemList buffer to blink::DisplayItem than to the
particular kind of item that caused new buffer allocation.

[jbroman, 2016-01-20 21:29:51]

On 2016/01/20 at 21:28:10, jbroman wrote:
> PTAL. I think this makes the integration a little less intrusive, and should
also make the resulting data clearer. For instance, it seems a little more
honest to attribute the DisplayItemList buffer to blink::DisplayItem than to the
particular kind of item that caused new buffer allocation.

(original patch that added this integration was
https://chromium.googlesource.com/chromium/src/+/006dc1e53075a580f789a2b18f7a...)

[Ruud van Asseldonk, 2016-01-21 10:23:23]

Thanks for doing this. I suppose you know better what the right type to report
is than I do. When I wrote this I added a |typeName| argument to all allocating
functions until at some point the type name was known.

Non-owner LGTM.

https://codereview.chromium.org/1609423002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/ContiguousContainer.h (right):

https://codereview.chromium.org/1609423002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/ContiguousContainer.h:157: return
*new (allocate(allocSize)) DerivedElementType(std::forward<Args>(args)...);
So now instead of reporting the type as |DerivedElementType|, it will be
reported as |BaseElementType|?

> For instance, it seems a little more honest to attribute
> the DisplayItemList buffer to blink::DisplayItem than to
> the particular kind of item that caused new buffer
> allocation.

Is this what you refer to here? I’m not familiar with how
|ContiguousContainerBase| is used in practice. If this makes type info more
honest then sure, go for it.

[jbroman, 2016-01-21 14:23:19]

tl;dr: The buffer can be shared by several subclasses of BaseElementType, since
they are allocated in large buffers instead of individually.

https://codereview.chromium.org/1609423002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/ContiguousContainer.h (right):

https://codereview.chromium.org/1609423002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/ContiguousContainer.h:157: return
*new (allocate(allocSize)) DerivedElementType(std::forward<Args>(args)...);
On 2016/01/21 at 10:23:23, Ruud van Asseldonk wrote:
> So now instead of reporting the type as |DerivedElementType|, it will be
reported as |BaseElementType|?

Right.

> > For instance, it seems a little more honest to attribute
> > the DisplayItemList buffer to blink::DisplayItem than to
> > the particular kind of item that caused new buffer
> > allocation.
> 
> Is this what you refer to here? I’m not familiar with how
|ContiguousContainerBase| is used in practice. If this makes type info more
honest then sure, go for it.

ContiguousContainer's actual user is blink::DisplayItemList, which is a
heterogeneous container of several subclasses of DisplayItem
(DrawingDisplayItem, BeginTransformDisplayItem, EndTransformDisplayItem, etc.).
ContiguousContainerBase allocates large buffers and then gives out contiguous
chunks of memory as items are added; a buffer may contain a variety of display
items.

The most that can be said of a buffer at allocation time in a DisplayItemList is
that it will contain DisplayItems. We happen to often know what the first item
in the buffer will be, but the buffer is not reserved for that type.

[jbroman, 2016-01-21 14:50:59]

The CQ bit was checked by jbroman@chromium.org

[commit-bot: I haz the power, 2016-01-21 14:51:09]

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

[Ruud van Asseldonk, 2016-01-21 14:51:53]

On 2016/01/21 14:23:19, jbroman wrote:
> tl;dr: The buffer can be shared by several subclasses of BaseElementType,
since
> they are allocated in large buffers instead of individually.
> 
>
https://codereview.chromium.org/1609423002/diff/1/third_party/WebKit/Source/p...
> File third_party/WebKit/Source/platform/graphics/ContiguousContainer.h
(right):
> 
>
https://codereview.chromium.org/1609423002/diff/1/third_party/WebKit/Source/p...
> third_party/WebKit/Source/platform/graphics/ContiguousContainer.h:157: return
> *new (allocate(allocSize)) DerivedElementType(std::forward<Args>(args)...);
> On 2016/01/21 at 10:23:23, Ruud van Asseldonk wrote:
> > So now instead of reporting the type as |DerivedElementType|, it will be
> reported as |BaseElementType|?
> 
> Right.
> 
> > > For instance, it seems a little more honest to attribute
> > > the DisplayItemList buffer to blink::DisplayItem than to
> > > the particular kind of item that caused new buffer
> > > allocation.
> > 
> > Is this what you refer to here? I’m not familiar with how
> |ContiguousContainerBase| is used in practice. If this makes type info more
> honest then sure, go for it.
> 
> ContiguousContainer's actual user is blink::DisplayItemList, which is a
> heterogeneous container of several subclasses of DisplayItem
> (DrawingDisplayItem, BeginTransformDisplayItem, EndTransformDisplayItem,
etc.).
> ContiguousContainerBase allocates large buffers and then gives out contiguous
> chunks of memory as items are added; a buffer may contain a variety of display
> items.
> 
> The most that can be said of a buffer at allocation time in a DisplayItemList
is
> that it will contain DisplayItems. We happen to often know what the first item
> in the buffer will be, but the buffer is not reserved for that type.

Ah, I see. Then it does make sense to blame the base type.

[commit-bot: I haz the power, 2016-01-21 16:18:54]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-01-21 16:20:07]

Description was changed from

==========
Adjust heap profiler integration for ContiguousContainer.

Firstly, change the type passed to the allocator to always be BaseElementType.
Otherwise, the buffer will be ascribed to whatever derived type was being
allocated when the buffer overflowed.

Secondly, confine the typeName parameter to only those places where the type
is passed from the ContiguousContainer template, through the base class, to
the allocator. (There are two entry points: initial capacity and new
allocation.)
==========

to

==========
Adjust heap profiler integration for ContiguousContainer.

Firstly, change the type passed to the allocator to always be BaseElementType.
Otherwise, the buffer will be ascribed to whatever derived type was being
allocated when the buffer overflowed.

Secondly, confine the typeName parameter to only those places where the type
is passed from the ContiguousContainer template, through the base class, to
the allocator. (There are two entry points: initial capacity and new
allocation.)

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

[commit-bot: I haz the power, 2016-01-21 16:20:07]

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