Oilpan: Support polymorphic objects in HeapVectorBackings

Oilpan: Support polymorphic objects in HeapVectorBackings

This CL supports HeapVector<T> where T is ALLOW_ONLY_INLINE_ALLOCATION and T has a vtable.

The way we support it is somewhat hacky:

1) VectorUnusedSlotClearer zeros out unused slots.

2) HeapVectorBacking calls finalizers and trace methods only for slots that are not zeroed out if T has a vtable. That way we can avoid HeapVectorBacking from visiting unused slots.

Note: For T that doesn't have a vtable, HeapVectorBacking visits all objects (including unused slots) treating a zeroed slot as a valid object. This works as long as T is canInitializeWithMemset. This means that after this CL, HeapVector can support objects that are canInitializeWithMemset or have a vtable. This CL adds a static_assert about it to Heap.h.

BUG=475801
TEST=HeapTest.VectorDestructorsWithVtable
TBR=tkent

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=194780

[haraken, 2015-04-24 10:06:57]

haraken@chromium.org changed reviewers:
+ oilpan-reviews@chromium.org

[haraken, 2015-04-24 10:06:57]

Not yet for review. I need to land https://codereview.chromium.org/1088973006/
first.

I understand this CL is doing a hacky thing, but it enables HeapVectorBackings
to hold objects that have vtables. Then we can remove OffHeapCollectionTrait.

[haraken, 2015-04-27 06:57:57]

PTAL

I agree that this is hacky, but I cannot come up with any alternative way to get
rid of OffHeapCollectionTrait.

https://codereview.chromium.org/1098953006/diff/20001/Source/wtf/Vector.h
File Source/wtf/Vector.h (right):

https://codereview.chromium.org/1098953006/diff/20001/Source/wtf/Vector.h#new...
Source/wtf/Vector.h:95: array[i] = 0;

We cannot now use memset here because clang complains that we should not use
memset for slots that contain vtables. Yeah, but we do want to do that here :)

[haraken, 2015-04-30 13:56:32]

The bot looks fine. Is anyone willing to look at this CL?

[sof, 2015-04-30 21:15:46]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2015-04-30 21:15:47]

lgtm to try (but non-owner for wtf/Vector.h)

https://codereview.chromium.org/1098953006/diff/20001/Source/platform/heap/He...
File Source/platform/heap/Heap.h (left):

https://codereview.chromium.org/1098953006/diff/20001/Source/platform/heap/He...
Source/platform/heap/Heap.h:2193: // HeapVectorBacking does not know the exact
size of the vector
Possible to have this long comment, which is very helpful & clear, not be
duplicated, but instead refer to the one next to trace() ?

https://codereview.chromium.org/1098953006/diff/20001/Source/wtf/Vector.h
File Source/wtf/Vector.h (right):

https://codereview.chromium.org/1098953006/diff/20001/Source/wtf/Vector.h#new...
Source/wtf/Vector.h:95: array[i] = 0;
On 2015/04/27 06:57:57, haraken wrote:
> 
> We cannot now use memset here because clang complains that we should not use
> memset for slots that contain vtables. Yeah, but we do want to do that here :)
> 

It might rewrite that loop to a memset() application internally -- I seem to
remember some amusing breakage we had in the past which ran into this (kouhei
fixed it, iirc.)

Wouldn't it work to do memset(reinterpret_cast<void*>(begin), ...) ?

[haraken, 2015-04-30 23:40:09]

haraken@chromium.org changed reviewers:
+ tkent@chromium.org

[haraken, 2015-04-30 23:40:20]

Thanks for review!

https://codereview.chromium.org/1098953006/diff/20001/Source/platform/heap/He...
File Source/platform/heap/Heap.h (left):

https://codereview.chromium.org/1098953006/diff/20001/Source/platform/heap/He...
Source/platform/heap/Heap.h:2193: // HeapVectorBacking does not know the exact
size of the vector
On 2015/04/30 21:15:47, sof wrote:
> Possible to have this long comment, which is very helpful & clear, not be
> duplicated, but instead refer to the one next to trace() ? 

Done.

https://codereview.chromium.org/1098953006/diff/20001/Source/wtf/Vector.h
File Source/wtf/Vector.h (right):

https://codereview.chromium.org/1098953006/diff/20001/Source/wtf/Vector.h#new...
Source/wtf/Vector.h:95: array[i] = 0;
On 2015/04/30 21:15:47, sof wrote:
> On 2015/04/27 06:57:57, haraken wrote:
> > 
> > We cannot now use memset here because clang complains that we should not use
> > memset for slots that contain vtables. Yeah, but we do want to do that here
:)
> > 
> 
> It might rewrite that loop to a memset() application internally -- I seem to
> remember some amusing breakage we had in the past which ran into this (kouhei
> fixed it, iirc.)
> 
> Wouldn't it work to do memset(reinterpret_cast<void*>(begin), ...) ? 

Thanks, it works.

[haraken, 2015-04-30 23:40:31]

TBRing tkent-san for wtf/.

[haraken, 2015-04-30 23:40:34]

The CQ bit was checked by haraken@chromium.org

[haraken, 2015-04-30 23:40:34]

The patchset sent to the CQ was uploaded after l-g-t-m from sigbjornf@opera.com
Link to the patchset: https://codereview.chromium.org/1098953006/#ps40001
(title: " ")

[commit-bot: I haz the power, 2015-04-30 23:40:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1098953006/40001

[commit-bot: I haz the power, 2015-05-01 01:06:21]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=194780

[tkent, 2015-05-07 00:43:21]

https://codereview.chromium.org/1098953006/diff/40001/Source/wtf/Vector.h
File Source/wtf/Vector.h (right):

https://codereview.chromium.org/1098953006/diff/40001/Source/wtf/Vector.h#new...
Source/wtf/Vector.h:93: memset(reinterpret_cast<void*>(begin), 0, sizeof(T) *
(end - begin));
Why do you do reinterpret_cast<>?

[haraken, 2015-05-07 00:49:41]

https://codereview.chromium.org/1098953006/diff/40001/Source/wtf/Vector.h
File Source/wtf/Vector.h (right):

https://codereview.chromium.org/1098953006/diff/40001/Source/wtf/Vector.h#new...
Source/wtf/Vector.h:93: memset(reinterpret_cast<void*>(begin), 0, sizeof(T) *
(end - begin));
On 2015/05/07 00:43:21, Slow until end of May wrote:
> Why do you do reinterpret_cast<>?

Because without the reinterpret_cast, clang complains that we should not use
memset for slots that contain vtables. We use this technique to avoid the
compile error in some places in platform/heap/.

[tkent, 2015-05-08 06:42:00]

https://codereview.chromium.org/1098953006/diff/40001/Source/wtf/Vector.h
File Source/wtf/Vector.h (right):

https://codereview.chromium.org/1098953006/diff/40001/Source/wtf/Vector.h#new...
Source/wtf/Vector.h:93: memset(reinterpret_cast<void*>(begin), 0, sizeof(T) *
(end - begin));
On 2015/05/07 00:49:40, haraken wrote:
> On 2015/05/07 00:43:21, Slow until end of May wrote:
> > Why do you do reinterpret_cast<>?
> 
> Because without the reinterpret_cast, clang complains that we should not use
> memset for slots that contain vtables. We use this technique to avoid the
> compile error in some places in platform/heap/.
> 

Thanks.  lgtm