Revert "Revert of [oilpan]: optimize the way we allocate persistent handles in wrappers. (patchset …

Revert "Revert of [oilpan]: optimize the way we allocate persistent handles in wrappers. (patchset #6 id:100001 of https://codereview.chromium.org/52535 3002/)"

Reland the wrapper persistent change using placement new for the constructor and no use of destructors. Also when constructing the object we make no assumption about the content of the slot.

This reverts commit 1738b5c841f11dd45dadefcd7b3e8ab6bab4eccb.

R=ager@chromium.org, fdegans@chromium.org, haraken@chromium.org, oilpan-reviews@chromium.org, zerny@chromium.org
BUG=411240
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181645

[haraken, 2014-09-09 11:22:12]

Would you just point out the diff from the previous CL?

[Mads Ager (chromium), 2014-09-09 11:29:23]

LGTM

https://codereview.chromium.org/556823003/diff/1/Source/platform/heap/Handle.h
File Source/platform/heap/Handle.h (right):

https://codereview.chromium.org/556823003/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:159: ALLOW_ONLY_INLINE_ALLOCATION()
Nit: Can we put a ';' at the end here. Makes it look a little more uniform and
some editors prefer to have it. :)

https://codereview.chromium.org/556823003/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:333: // We use placement new to call the
constructor to ensure that we setup vtable correctly.
vtable -> the vtable

[wibling-chromium, 2014-09-09 11:32:48]

@haraken

Please see comments below for the major changes.

https://codereview.chromium.org/556823003/diff/1/Source/platform/heap/Handle.cpp
File Source/platform/heap/Handle.cpp (right):

https://codereview.chromium.org/556823003/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.cpp:57: Address
WrapperPersistentRegion::outOfLineAllocate(ThreadState* state,
WrapperPersistentRegion** regionPtr)
We pass in the regionPtr pointing to the last region where the caller tried
allocating a persistent. The callee (outOfLineAllocate) updates this with the
region where the persistent ended up being allocated.
This is used to compute the correct regionOffset in the create method.

https://codereview.chromium.org/556823003/diff/1/Source/platform/heap/Handle.h
File Source/platform/heap/Handle.h (right):

https://codereview.chromium.org/556823003/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:159: ALLOW_ONLY_INLINE_ALLOCATION()
Disallow dynamic allocation. We only allow placement new.

https://codereview.chromium.org/556823003/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:172: static inline void destroy(const
WrapperPersistentNode*);
Use destroy(...) instead of calling delete on the WrapperPersistents.

https://codereview.chromium.org/556823003/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:176: WrapperPersistentNode(void *raw, size_t
regionOffset) : m_raw(raw), m_regionOffset(regionOffset) { }
New constructor taking the regionOffset to ensure we don't rely on it being
unchanged.

https://codereview.chromium.org/556823003/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:198: void operator delete(void*);
Ensure no one calls delete.

https://codereview.chromium.org/556823003/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:215: ALLOW_ONLY_INLINE_ALLOCATION()
Only allow placement new.

https://codereview.chromium.org/556823003/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:217: static WrapperPersistent<T>* create(T* raw);
Allocate WrapperPersistent<T> via the create method not via new.

https://codereview.chromium.org/556823003/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:230: WrapperPersistent(T* raw, size_t
regionOffset) : WrapperPersistentNode(raw, regionOffset) { }
New constructor taking both the raw pointer and the region offset to ensure we
don't rely on the state being unmodified by the allocator.

https://codereview.chromium.org/556823003/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:233: void operator delete(void*);
Disallow delete.

https://codereview.chromium.org/556823003/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:319: WrapperPersistent<T>*
WrapperPersistent<T>::create(T* raw)
New create and destroy...

[zerny-chromium, 2014-09-09 11:33:11]

lgtm with some optional nits

https://codereview.chromium.org/556823003/diff/1/Source/platform/heap/Handle.h
File Source/platform/heap/Handle.h (right):

https://codereview.chromium.org/556823003/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:162: bool isAlive() { return m_regionOffset &
wrapperPersistentLiveBitMask; }
Nit: I'd add the corresponding accessor for:

  size_t offset() { ... }

and use that below.

https://codereview.chromium.org/556823003/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:207: // WrapperPersistentRegion.
The m_regionOffset field encodes the liveness of the node and the offset ...

https://codereview.chromium.org/556823003/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:219: virtual void trace(Visitor* visitor)
Nit: OVERRIDE

https://codereview.chromium.org/556823003/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:331: regionOffset |= wrapperPersistentLiveBitMask;
Nit: Would it not be more natural to have this OR in the initialization list of
the constructor of WrapperPersistentNode?

[haraken, 2014-09-09 11:42:48]

LGTM, thanks!

[wibling-chromium, 2014-09-09 11:49:50]

Thanks for the reviews!

https://codereview.chromium.org/556823003/diff/1/Source/platform/heap/Handle.h
File Source/platform/heap/Handle.h (right):

https://codereview.chromium.org/556823003/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:159: ALLOW_ONLY_INLINE_ALLOCATION()
On 2014/09/09 11:29:23, Mads Ager (chromium) wrote:
> Nit: Can we put a ';' at the end here. Makes it look a little more uniform and
> some editors prefer to have it. :)

Done.

https://codereview.chromium.org/556823003/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:162: bool isAlive() { return m_regionOffset &
wrapperPersistentLiveBitMask; }
On 2014/09/09 11:33:11, zerny-chromium wrote:
> Nit: I'd add the corresponding accessor for:
> 
>   size_t offset() { ... }
> 
> and use that below.

Done.

https://codereview.chromium.org/556823003/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:207: // WrapperPersistentRegion.
On 2014/09/09 11:33:11, zerny-chromium wrote:
> The m_regionOffset field encodes the liveness of the node and the offset ...

Done.

https://codereview.chromium.org/556823003/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:219: virtual void trace(Visitor* visitor)
On 2014/09/09 11:33:11, zerny-chromium wrote:
> Nit: OVERRIDE

Done.

https://codereview.chromium.org/556823003/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:331: regionOffset |= wrapperPersistentLiveBitMask;
On 2014/09/09 11:33:11, zerny-chromium wrote:
> Nit: Would it not be more natural to have this OR in the initialization list
of
> the constructor of WrapperPersistentNode?

I can't do that as the code is now, since the constructor is also used to
initialize the entries when first allocated where the bit shouldn't be set.

https://codereview.chromium.org/556823003/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:333: // We use placement new to call the
constructor to ensure that we setup vtable correctly.
On 2014/09/09 11:29:23, Mads Ager (chromium) wrote:
> vtable -> the vtable

Done.

[Fabrice (no longer in Chrome), 2014-09-09 12:16:05]

LGTM, on the condition android_dbg_tests_recipe passes.
I tested it locally and the tests previously failing with GCC 4.9 now pass so it
should be good!

[wibling-chromium, 2014-09-09 13:11:47]

On 2014/09/09 12:16:05, Fabrice de Gans wrote:
> LGTM, on the condition android_dbg_tests_recipe passes.
> I tested it locally and the tests previously failing with GCC 4.9 now pass so
it
> should be good!

Thanks for verifying.

[wibling-chromium, 2014-09-09 13:11:53]

The CQ bit was checked by wibling@chromium.org

[commit-bot: I haz the power, 2014-09-09 13:12:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wibling@chromium.org/556823003/20001

[commit-bot: I haz the power, 2014-09-09 14:03:52]

Committed patchset #2 (id:20001) as 181645