Oilpan: Correct static_asserts about VectorTraits::canInitializeWithMemset

Oilpan: Correct static_asserts about VectorTraits::canInitializeWithMemset

HeapVectorBacking::trace and HeapVectorBacking::finalize do not support items that cannot be initialized with memset. This CL adds static_asserts about the condition.

This CL also makes the following change:

- Remove a static_assert about 'sizeof(T) > allocationGranularity', since the assert doesn't make sense. Even if sizeof(T) is smaller than the allocationGranurarity (i.e., 8 byte), HeapVectorBacking::trace/finalize should work correctly as long as the object can be initialized with memset.

- Remove static_asserts from Vector.h, since the static_asserts in Heap.h are enough.

- Add a WTF_INIT_WITH_MEM_FUNCTIONS macro to RuleFeature and UpdatedAnimationStyle. The macros were just missing.

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

[haraken, 2015-04-22 16:10:33]

haraken@chromium.org changed reviewers:
+ oilpan-reviews@chromium.org, sigbjornf@opera.com

[haraken, 2015-04-22 16:10:33]

This CL doesn't yet compile since there remain a bunch of vectors that don't
have canInitializeWithMemset...

[haraken, 2015-04-22 16:13:22]

https://codereview.chromium.org/1088973006/diff/1/Source/platform/heap/Heap.h
File Source/platform/heap/Heap.h (left):

https://codereview.chromium.org/1088973006/diff/1/Source/platform/heap/Heap.h...
Source/platform/heap/Heap.h:1930: static_assert(!ShouldBeTraced<Traits>::value
|| sizeof(T) > blink::allocationGranularity || Traits::canInitializeWithMemset,
"heap overallocation can cause spurious visits");

Do you know why we need to check 'sizeof(T) > allocationGranularity'? The above
comment is not clear enough to convince me that the condition is necessary to
safely trace the zeroed-out memory.

[haraken, 2015-04-22 16:16:48]

The patch set 2 compiles but is wrong.

https://codereview.chromium.org/1088973006/diff/20001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLVertexArrayObjectOES.h (right):

https://codereview.chromium.org/1088973006/diff/20001/Source/core/html/canvas...
Source/core/html/canvas/WebGLVertexArrayObjectOES.h:116:
WTF_ALLOW_MOVE_AND_INIT_WITH_MEM_FUNCTIONS(blink::WebGLVertexArrayObjectOES::VertexAttribState);

This is a wrong change. As commented in the previous code, VertexAttribState
needs to use a constructor.

Maybe do we need to make VertexAttribState GarbageCollected?

[haraken, 2015-04-23 08:42:38]

PTAL

See the CL description for the rationale of this change.

[sof, 2015-04-23 08:46:00]

On 2015/04/23 08:42:38, haraken wrote:
> PTAL
> 
> See the CL description for the rationale of this change.

I don't understand why it is safe to initialize (e.g.) VertexAttribState::state
as 0 via memset() rather than 4 (which is what the constructor does.)

[haraken, 2015-04-23 08:55:48]

On 2015/04/23 08:46:00, sof wrote:
> On 2015/04/23 08:42:38, haraken wrote:
> > PTAL
> > 
> > See the CL description for the rationale of this change.
> 
> I don't understand why it is safe to initialize (e.g.)
VertexAttribState::state
> as 0 via memset() rather than 4 (which is what the constructor does.)

As far as I'm understanding things correctly, "canInitializeWithMemset=true"
doesn't mean that a zeroed object is a semantically valid object.
"canInitializeWithMemset=true" means that it is safe to call a trace method and
a finalizer of a zeroed object. (Probably we should rename
"canInitializeWithMemset" to a better name.)

[sof, 2015-04-23 08:59:18]

On 2015/04/23 08:55:48, haraken wrote:
> On 2015/04/23 08:46:00, sof wrote:
> > On 2015/04/23 08:42:38, haraken wrote:
> > > PTAL
> > > 
> > > See the CL description for the rationale of this change.
> > 
> > I don't understand why it is safe to initialize (e.g.)
> VertexAttribState::state
> > as 0 via memset() rather than 4 (which is what the constructor does.)
> 
> As far as I'm understanding things correctly, "canInitializeWithMemset=true"
> doesn't mean that a zeroed object is a semantically valid object.
> "canInitializeWithMemset=true" means that it is safe to call a trace method
and
> a finalizer of a zeroed object. (Probably we should rename
> "canInitializeWithMemset" to a better name.)

It controls VectorInializer<>, so if you have an inline vector buffer and then
resize() it, wouldn't the entries become initialized with memset() ?

[haraken, 2015-04-23 10:27:49]

On 2015/04/23 08:59:18, sof wrote:
> On 2015/04/23 08:55:48, haraken wrote:
> > On 2015/04/23 08:46:00, sof wrote:
> > > On 2015/04/23 08:42:38, haraken wrote:
> > > > PTAL
> > > > 
> > > > See the CL description for the rationale of this change.
> > > 
> > > I don't understand why it is safe to initialize (e.g.)
> > VertexAttribState::state
> > > as 0 via memset() rather than 4 (which is what the constructor does.)
> > 
> > As far as I'm understanding things correctly, "canInitializeWithMemset=true"
> > doesn't mean that a zeroed object is a semantically valid object.
> > "canInitializeWithMemset=true" means that it is safe to call a trace method
> and
> > a finalizer of a zeroed object. (Probably we should rename
> > "canInitializeWithMemset" to a better name.)
> 
> It controls VectorInializer<>, so if you have an inline vector buffer and then
> resize() it, wouldn't the entries become initialized with memset() ?

Good point, you're right.

It seems we need to introduce canCallTraceAndFinalizerOnZeroedMemory but we
won't want to introduce such a thing. Do you think it would be ok to make
VertexAttribState GarbageCollected (in terms of performance)?

[sof, 2015-04-23 11:29:56]

On 2015/04/23 10:27:49, haraken wrote:
> On 2015/04/23 08:59:18, sof wrote:
> > On 2015/04/23 08:55:48, haraken wrote:
> > > On 2015/04/23 08:46:00, sof wrote:
> > > > On 2015/04/23 08:42:38, haraken wrote:
> > > > > PTAL
> > > > > 
> > > > > See the CL description for the rationale of this change.
> > > > 
> > > > I don't understand why it is safe to initialize (e.g.)
> > > VertexAttribState::state
> > > > as 0 via memset() rather than 4 (which is what the constructor does.)
> > > 
> > > As far as I'm understanding things correctly,
"canInitializeWithMemset=true"
> > > doesn't mean that a zeroed object is a semantically valid object.
> > > "canInitializeWithMemset=true" means that it is safe to call a trace
method
> > and
> > > a finalizer of a zeroed object. (Probably we should rename
> > > "canInitializeWithMemset" to a better name.)
> > 
> > It controls VectorInializer<>, so if you have an inline vector buffer and
then
> > resize() it, wouldn't the entries become initialized with memset() ?
> 
> Good point, you're right.
> 
> It seems we need to introduce canCallTraceAndFinalizerOnZeroedMemory but we
> won't want to introduce such a thing. Do you think it would be ok to make
> VertexAttribState GarbageCollected (in terms of performance)?

It ought to be ok, the size of the vector holding them isn't substantial (16 is
common), but you'd have to check with WebGL-focused folks.

What goes wrong on trunk for VertexAttribState & its current vector traits?
I think the maxiumum number of such attributes is quite low (16 is common), so I
wouldn't expect it to be

[haraken, 2015-04-23 11:52:57]

> What goes wrong on trunk for VertexAttribState & its current vector traits?

HeapVectorBacking doesn't support objects that cannot be initialized with
memset; i.e., canInitializeWithMemset needs to be true.

The reason it was working before this CL is the static_assert in Heap.h was
wrong. (It was meeting 'sizeof(T) > allocationGranularity' and bypassing the
'canInitializeWithMemset' check.)

[sof, 2015-04-24 12:25:36]

On 2015/04/23 11:52:57, haraken wrote:
> > What goes wrong on trunk for VertexAttribState & its current vector traits?
> 
> HeapVectorBacking doesn't support objects that cannot be initialized with
> memset; i.e., canInitializeWithMemset needs to be true.
> 
> The reason it was working before this CL is the static_assert in Heap.h was
> wrong. (It was meeting 'sizeof(T) > allocationGranularity' and bypassing the
> 'canInitializeWithMemset' check.)

Thanks for the explanation of why; lgtm.

[sof, 2015-04-24 12:26:10]

(needs a rebase for VertexAttribState, ofc.)

https://codereview.chromium.org/1088973006/diff/40001/Source/platform/heap/He...
File Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/1088973006/diff/40001/Source/platform/heap/He...
Source/platform/heap/Heap.h:2216: static_assert(Traits::needsDestruction,
"HeapVectors that don't require destructors should not reach here.");
"Only vector buffers with items requiring destruction should be finalized" ?
(i.e., deques are backed by same.)

[haraken, 2015-04-24 12:34:16]

Thanks for review!

>
https://codereview.chromium.org/1088973006/diff/40001/Source/platform/heap/He...
> File Source/platform/heap/Heap.h (right):
> 
>
https://codereview.chromium.org/1088973006/diff/40001/Source/platform/heap/He...
> Source/platform/heap/Heap.h:2216: static_assert(Traits::needsDestruction,
> "HeapVectors that don't require destructors should not reach here.");
> "Only vector buffers with items requiring destruction should be finalized" ?
> (i.e., deques are backed by same.)

Done. Also updated some other comments.

[haraken, 2015-04-24 12:35:20]

On 2015/04/24 12:34:16, haraken wrote:
> Thanks for review!
> 
> >
>
https://codereview.chromium.org/1088973006/diff/40001/Source/platform/heap/He...
> > File Source/platform/heap/Heap.h (right):
> > 
> >
>
https://codereview.chromium.org/1088973006/diff/40001/Source/platform/heap/He...
> > Source/platform/heap/Heap.h:2216: static_assert(Traits::needsDestruction,
> > "HeapVectors that don't require destructors should not reach here.");
> > "Only vector buffers with items requiring destruction should be finalized" ?
> > (i.e., deques are backed by same.)
> 
> Done. Also updated some other comments.

Also I removed the assert in Vector.h, since the static_asserts in Heap.h are
enough. Does this look OK?

[haraken, 2015-04-24 12:36:17]

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

[haraken, 2015-04-24 12:36:18]

tkent-san: Mind taking a look at wtf/?

[sof, 2015-04-24 12:40:06]

https://codereview.chromium.org/1088973006/diff/80001/Source/wtf/Vector.h
File Source/wtf/Vector.h (left):

https://codereview.chromium.org/1088973006/diff/80001/Source/wtf/Vector.h#old...
Source/wtf/Vector.h:643: static_assert(!WTF::IsPolymorphic<T>::value ||
!VectorTraits<T>::canInitializeWithMemset, "cannot initialize with memset if
there is a vtable");
Doesn't this check help catch incorrect trait specializations?

[haraken, 2015-04-24 12:45:37]

https://codereview.chromium.org/1088973006/diff/80001/Source/wtf/Vector.h
File Source/wtf/Vector.h (left):

https://codereview.chromium.org/1088973006/diff/80001/Source/wtf/Vector.h#old...
Source/wtf/Vector.h:643: static_assert(!WTF::IsPolymorphic<T>::value ||
!VectorTraits<T>::canInitializeWithMemset, "cannot initialize with memset if
there is a vtable");
On 2015/04/24 12:40:06, sof wrote:
> Doesn't this check help catch incorrect trait specializations?

Doesn't the check in HeapVectorBacking::trace and HeapVectorBacking::finalize
imply the check here?

[sof, 2015-04-24 12:50:39]

https://codereview.chromium.org/1088973006/diff/80001/Source/wtf/Vector.h
File Source/wtf/Vector.h (left):

https://codereview.chromium.org/1088973006/diff/80001/Source/wtf/Vector.h#old...
Source/wtf/Vector.h:643: static_assert(!WTF::IsPolymorphic<T>::value ||
!VectorTraits<T>::canInitializeWithMemset, "cannot initialize with memset if
there is a vtable");
On 2015/04/24 12:45:37, haraken wrote:
> On 2015/04/24 12:40:06, sof wrote:
> > Doesn't this check help catch incorrect trait specializations?
> 
> Doesn't the check in HeapVectorBacking::trace and HeapVectorBacking::finalize
> imply the check here?

Not quite, they trust canInitializeWithMemset is correct for a class type (as
given by a specialization, quite possibly.)

[haraken, 2015-04-24 13:02:26]

https://codereview.chromium.org/1088973006/diff/80001/Source/wtf/Vector.h
File Source/wtf/Vector.h (left):

https://codereview.chromium.org/1088973006/diff/80001/Source/wtf/Vector.h#old...
Source/wtf/Vector.h:643: static_assert(!WTF::IsPolymorphic<T>::value ||
!VectorTraits<T>::canInitializeWithMemset, "cannot initialize with memset if
there is a vtable");
On 2015/04/24 12:50:39, sof wrote:
> On 2015/04/24 12:45:37, haraken wrote:
> > On 2015/04/24 12:40:06, sof wrote:
> > > Doesn't this check help catch incorrect trait specializations?
> > 
> > Doesn't the check in HeapVectorBacking::trace and
HeapVectorBacking::finalize
> > imply the check here?
> 
> Not quite, they trust canInitializeWithMemset is correct for a class type (as
> given by a specialization, quite possibly.)

Makes sense. So...

- We don't need the first static_assert.

- We need the static_assert(!WTF::IsPolymorphic<T>::value || ...) here.

- We also need the static_assert(!WTF::IsPolymorphic<T>::value || ...) in
Vector(size_t).

Right?

[sof, 2015-04-24 13:11:08]

https://codereview.chromium.org/1088973006/diff/80001/Source/wtf/Vector.h
File Source/wtf/Vector.h (left):

https://codereview.chromium.org/1088973006/diff/80001/Source/wtf/Vector.h#old...
Source/wtf/Vector.h:643: static_assert(!WTF::IsPolymorphic<T>::value ||
!VectorTraits<T>::canInitializeWithMemset, "cannot initialize with memset if
there is a vtable");
On 2015/04/24 13:02:26, haraken wrote:
> On 2015/04/24 12:50:39, sof wrote:
> > On 2015/04/24 12:45:37, haraken wrote:
> > > On 2015/04/24 12:40:06, sof wrote:
> > > > Doesn't this check help catch incorrect trait specializations?
> > > 
> > > Doesn't the check in HeapVectorBacking::trace and
> HeapVectorBacking::finalize
> > > imply the check here?
> > 
> > Not quite, they trust canInitializeWithMemset is correct for a class type
(as
> > given by a specialization, quite possibly.)
> 
> Makes sense. So...
> 
> - We don't need the first static_assert.
> 
> - We need the static_assert(!WTF::IsPolymorphic<T>::value || ...) here.
> 
> - We also need the static_assert(!WTF::IsPolymorphic<T>::value || ...) in
> Vector(size_t).
> 
> Right?
> 
> 

That makes sense to me, the more helpful static_assert()s we can provide, the
better in this area imho.

[haraken, 2015-04-24 13:13:48]

https://codereview.chromium.org/1088973006/diff/80001/Source/wtf/Vector.h
File Source/wtf/Vector.h (left):

https://codereview.chromium.org/1088973006/diff/80001/Source/wtf/Vector.h#old...
Source/wtf/Vector.h:643: static_assert(!WTF::IsPolymorphic<T>::value ||
!VectorTraits<T>::canInitializeWithMemset, "cannot initialize with memset if
there is a vtable");
On 2015/04/24 13:11:07, sof wrote:
> On 2015/04/24 13:02:26, haraken wrote:
> > On 2015/04/24 12:50:39, sof wrote:
> > > On 2015/04/24 12:45:37, haraken wrote:
> > > > On 2015/04/24 12:40:06, sof wrote:
> > > > > Doesn't this check help catch incorrect trait specializations?
> > > > 
> > > > Doesn't the check in HeapVectorBacking::trace and
> > HeapVectorBacking::finalize
> > > > imply the check here?
> > > 
> > > Not quite, they trust canInitializeWithMemset is correct for a class type
> (as
> > > given by a specialization, quite possibly.)
> > 
> > Makes sense. So...
> > 
> > - We don't need the first static_assert.
> > 
> > - We need the static_assert(!WTF::IsPolymorphic<T>::value || ...) here.
> > 
> > - We also need the static_assert(!WTF::IsPolymorphic<T>::value || ...) in
> > Vector(size_t).
> > 
> > Right?
> > 
> > 
> 
> That makes sense to me, the more helpful static_assert()s we can provide, the
> better in this area imho.

Done.

[haraken, 2015-04-27 03:14:59]

tkent-san: Mind taking a look at wtf/?

[tkent, 2015-04-27 03:29:23]

lgtm

https://codereview.chromium.org/1088973006/diff/100001/Source/platform/heap/H...
File Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/1088973006/diff/100001/Source/platform/heap/H...
Source/platform/heap/Heap.h:1899: // (this is done by VectorUnusedSlotClearer)
and T can be initialized
Please document this in http://dev.chromium.org/blink/blink-gc .

[haraken, 2015-04-27 03:54:41]

https://codereview.chromium.org/1088973006/diff/100001/Source/platform/heap/H...
File Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/1088973006/diff/100001/Source/platform/heap/H...
Source/platform/heap/Heap.h:1899: // (this is done by VectorUnusedSlotClearer)
and T can be initialized
On 2015/04/27 03:29:23, tkent wrote:
> Please document this in http://dev.chromium.org/blink/blink-gc .

Done.

[haraken, 2015-04-27 03:54:46]

The CQ bit was checked by haraken@chromium.org

[haraken, 2015-04-27 03:54:47]

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/1088973006/#ps100001
(title: " ")

[commit-bot: I haz the power, 2015-04-27 03:54:56]

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

[commit-bot: I haz the power, 2015-04-27 05:11:32]

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=194480