[oilpan]: optimize the way we allocate persistent handles in wrappers.

[oilpan]: optimize the way we allocate persistent handles in wrappers.

The oilpan JS wrappers all have a persistent which is used to keep the wrapped object around while there is a javascript reference to it. Allocation of these wrapper persistents showed up in some of benchmarks. This change creates a new optimized WrapperPersistent which is allocated in blocks. This gives a speedup ranging between a few percent to 20% in some of the more wrapper intensive tests.

R=ager@chromium.org, haraken@chromium.org, oilpan-reviews@chromium.org, zerny@chromium.org
BUG=

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

[Erik Corry, 2014-09-01 13:40:40]

erik.corry@gmail.com changed reviewers:
+ erik.corry@gmail.com

[Erik Corry, 2014-09-01 13:40:41]

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

https://codereview.chromium.org/525353002/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:237: void* allocate()
I think you want this to be inlined, and you expect the 'if' to be almost always
true.

I would suggest

if (!m_free)
  return out_of_line_function(); // Define this in the C++ file so it is not
inlined.
...

[Mads Ager (chromium), 2014-09-01 13:52:46]

https://codereview.chromium.org/525353002/diff/1/Source/core/inspector/Inject...
File Source/core/inspector/InjectedScriptManager.cpp (right):

https://codereview.chromium.org/525353002/diff/1/Source/core/inspector/Inject...
Source/core/inspector/InjectedScriptManager.cpp:48: if (hostPtr)
No need to check, delete ignores null ptrs.

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

https://codereview.chromium.org/525353002/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:199: void* m_raw;
Add a comment that m_raw is used to chain freelist entries? Maybe add some
helper methods for when m_raw is used as a freelist entry to make the code
easier to read: setNextFree, nextFree?

https://codereview.chromium.org/525353002/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:226: for (int i = wrapperPersistentsPerRegion-1; i
>= 0; --i) {
Nit: space around '-'

https://codereview.chromium.org/525353002/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:242: m_free =
reinterpret_cast<WrapperPersistentNode*>(free->m_raw);
Maybe wrap this in a method: free->nextFree().

Then we can have an assert in nextFree that the live bit is not set on the
WrapperPersistentNode* that is returned?

https://codereview.chromium.org/525353002/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:249: return m_next->allocate();
Can we use iteration iteration from the outside instead of recursion? Maybe I
should trust the compiler more to do tail-call optimization?

https://codereview.chromium.org/525353002/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:274: m_next->trace(visitor);
Can we use iteration from the outside instead of recursion?

for (WrapperPersistentRegion* current = ...; current; current = current->next())
{
  current->trace();
}

With a block size of 256 we can probably get a lot of blocks (at least on
artificial benchmarks). Maybe I'm too old fashioned. I'm worried that the
compiler will not perform tail call optimization here and that we have a risk of
blowing out the stack.

https://codereview.chromium.org/525353002/diff/1/Source/platform/heap/ThreadS...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/525353002/diff/1/Source/platform/heap/ThreadS...
Source/platform/heap/ThreadState.cpp:335: while (m_wrapperPersistents) {
How about regions that are in the pool? Should we delete those here as well?

[zerny-chromium, 2014-09-01 14:13:37]

A quick comment in passing.

https://codereview.chromium.org/525353002/diff/1/Source/bindings/core/v8/cust...
File Source/bindings/core/v8/custom/V8InjectedScriptManager.cpp (right):

https://codereview.chromium.org/525353002/diff/1/Source/bindings/core/v8/cust...
Source/bindings/core/v8/custom/V8InjectedScriptManager.cpp:78:
callbackData->host = host.get();
Is this field never used in the oilpan build?

https://codereview.chromium.org/525353002/diff/1/Source/core/inspector/Inject...
File Source/core/inspector/InjectedScriptManager.h (right):

https://codereview.chromium.org/525353002/diff/1/Source/core/inspector/Inject...
Source/core/inspector/InjectedScriptManager.h:53:
WrapperPersistent<InjectedScriptHost>* hostPtr;
Why the name change?

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

https://codereview.chromium.org/525353002/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:188: m_offset |= wrapperPersistentLiveBitMask;
m_offset is uninitialized here. Maybe just m_offset = wrapper...

[haraken, 2014-09-02 05:22:06]

Looks like a nice change.

https://codereview.chromium.org/525353002/diff/1/Source/core/inspector/Inject...
File Source/core/inspector/InjectedScriptManager.h (right):

https://codereview.chromium.org/525353002/diff/1/Source/core/inspector/Inject...
Source/core/inspector/InjectedScriptManager.h:53:
WrapperPersistent<InjectedScriptHost>* hostPtr;

InjectedScriptManager is a very special case, so you don't need to change this
in this CL. I'm planning to replace the CallbackData with
DOMWrapperWorld::DOMObjectHolder in the near future.

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

https://codereview.chromium.org/525353002/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:180: virtual void trace(Visitor* visitor) { }

You won't need this. You can just define non-virtual trace method in
WrapperPersistent.

https://codereview.chromium.org/525353002/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:186: {

Can we add ASSERT(!m_raw) ?

https://codereview.chromium.org/525353002/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:202: size_t m_offset;

m_offset => m_region or m_regionHead

https://codereview.chromium.org/525353002/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:202: size_t m_offset;

Add a comment about the memory structure of m_offset.

https://codereview.chromium.org/525353002/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:208: class WrapperPersistent : public
WrapperPersistentNode {

Add FINAL.

https://codereview.chromium.org/525353002/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:279: bool
removeIfNotLast(WrapperPersistentRegion** headPtr)

Shall we move not-performance-sensitive methods to cpp files? It's not nice to
increase Handle.h :)

https://codereview.chromium.org/525353002/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:285: if (m_prev) {

Can we just call removeHead() here?

https://codereview.chromium.org/525353002/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:327: WrapperPersistentRegion* m_next;

Shall we override 'operator new=' in a private section? If it's used, we'll end
up breaking the doubly-linked list unexpectedly.

https://codereview.chromium.org/525353002/diff/1/Source/platform/heap/ThreadS...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/525353002/diff/1/Source/platform/heap/ThreadS...
Source/platform/heap/ThreadState.cpp:710: // Region was removed, ie. it is was
not the last region in the list.

it is was => it was

https://codereview.chromium.org/525353002/diff/1/Source/platform/heap/ThreadS...
Source/platform/heap/ThreadState.cpp:711: if (m_wrapperPersistentRegionPoolSize
< 2) {

What does the '< 2' check mean?

https://codereview.chromium.org/525353002/diff/1/Source/platform/heap/ThreadS...
File Source/platform/heap/ThreadState.h (right):

https://codereview.chromium.org/525353002/diff/1/Source/platform/heap/ThreadS...
Source/platform/heap/ThreadState.h:664: WrapperPersistentRegion*
m_wrapperPersistents;

m_wrapperPersistents => m_usedWrapperPersistents
m_wrapperPersistentRegionPool => m_pooledWrapperPersistents

?

https://codereview.chromium.org/525353002/diff/1/Source/platform/heap/ThreadS...
Source/platform/heap/ThreadState.h:665: WrapperPersistentRegion*
m_wrapperPersistentRegionPool;

WrapperPersistentRegion is owned by either m_wrapperPersistents or
m_wrapperPersistentRegionPool. Can we use Deque<OwnPtr<WrapperPersistentRegion>>
and avoid implementing insertHead, removeHead etc?

[wibling-chromium, 2014-09-02 11:19:38]

Thanks for the reviews!
I have updated the patch with the feedback and replied inline below.

https://codereview.chromium.org/525353002/diff/1/Source/bindings/core/v8/cust...
File Source/bindings/core/v8/custom/V8InjectedScriptManager.cpp (right):

https://codereview.chromium.org/525353002/diff/1/Source/bindings/core/v8/cust...
Source/bindings/core/v8/custom/V8InjectedScriptManager.cpp:78:
callbackData->host = host.get();
On 2014/09/01 14:13:37, zerny-chromium wrote:
> Is this field never used in the oilpan build?

No, AFAICT it is only used to keep the object alive.

https://codereview.chromium.org/525353002/diff/1/Source/core/inspector/Inject...
File Source/core/inspector/InjectedScriptManager.cpp (right):

https://codereview.chromium.org/525353002/diff/1/Source/core/inspector/Inject...
Source/core/inspector/InjectedScriptManager.cpp:48: if (hostPtr)
On 2014/09/01 13:52:45, Mads Ager (chromium) wrote:
> No need to check, delete ignores null ptrs.

Done.

https://codereview.chromium.org/525353002/diff/1/Source/core/inspector/Inject...
File Source/core/inspector/InjectedScriptManager.h (right):

https://codereview.chromium.org/525353002/diff/1/Source/core/inspector/Inject...
Source/core/inspector/InjectedScriptManager.h:53:
WrapperPersistent<InjectedScriptHost>* hostPtr;
On 2014/09/01 14:13:37, zerny-chromium wrote:
> Why the name change?

Since it it now a pointer to the WrapperPersistent rather than an embedded
RefPtr.

https://codereview.chromium.org/525353002/diff/1/Source/core/inspector/Inject...
Source/core/inspector/InjectedScriptManager.h:53:
WrapperPersistent<InjectedScriptHost>* hostPtr;
On 2014/09/02 05:22:05, haraken wrote:
> 
> InjectedScriptManager is a very special case, so you don't need to change this
> in this CL. I'm planning to replace the CallbackData with
> DOMWrapperWorld::DOMObjectHolder in the near future.

If i don't change it I cannot change the setXXX methods to consistently use a
WrapperPersistentNode since this will still use a normal Persistent.

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

https://codereview.chromium.org/525353002/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:180: virtual void trace(Visitor* visitor) { }
On 2014/09/02 05:22:05, haraken wrote:
> 
> You won't need this. You can just define non-virtual trace method in
> WrapperPersistent.

I do need this, since I am calling m_entries[i].trace(visitor) which is a
WrapperPersistentNode and I want it to dispatch to the instantiated
WrapperPersistent<T> subclass in order to cast it to the correct type before
calling mark.

https://codereview.chromium.org/525353002/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:186: {
On 2014/09/02 05:22:05, haraken wrote:
> 
> Can we add ASSERT(!m_raw) ?

Done.

https://codereview.chromium.org/525353002/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:188: m_offset |= wrapperPersistentLiveBitMask;
On 2014/09/01 14:13:37, zerny-chromium wrote:
> m_offset is uninitialized here. Maybe just m_offset = wrapper...

m_offset has already been initialized here to have an offset to the base of the
region. This happens in the WrapperPersistentRegion's constructor which calls
the below constructor to set up the fixed offset and the pointer to the next
free entry.
I agree this is a bit confusing, ie. that we essentially call a constructor to
setup free entry and a second constructor when the entry is used by a wrapper. I
have changed it such that the WrapperPersistentRegion's constructor calls an
initSlot method to setup the entry and the constructor is only called when the
entry is allocated/constructed by a wrapper.

https://codereview.chromium.org/525353002/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:199: void* m_raw;
On 2014/09/01 13:52:46, Mads Ager (chromium) wrote:
> Add a comment that m_raw is used to chain freelist entries? Maybe add some
> helper methods for when m_raw is used as a freelist entry to make the code
> easier to read: setNextFree, nextFree?

Done.

https://codereview.chromium.org/525353002/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:202: size_t m_offset;
On 2014/09/02 05:22:05, haraken wrote:
> 
> m_offset => m_region or m_regionHead

Changed it to m_regionOffset.

https://codereview.chromium.org/525353002/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:202: size_t m_offset;
On 2014/09/02 05:22:05, haraken wrote:
> 
> Add a comment about the memory structure of m_offset.

Done.

https://codereview.chromium.org/525353002/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:208: class WrapperPersistent : public
WrapperPersistentNode {
On 2014/09/02 05:22:05, haraken wrote:
> 
> Add FINAL.

Done.

https://codereview.chromium.org/525353002/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:226: for (int i = wrapperPersistentsPerRegion-1; i
>= 0; --i) {
On 2014/09/01 13:52:46, Mads Ager (chromium) wrote:
> Nit: space around '-'

Done.

https://codereview.chromium.org/525353002/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:237: void* allocate()
On 2014/09/01 13:40:41, Erik Corry wrote:
> I think you want this to be inlined, and you expect the 'if' to be almost
always
> true.
> 
> I would suggest
> 
> if (!m_free)
>   return out_of_line_function(); // Define this in the C++ file so it is not
> inlined.
> ...

Done.

https://codereview.chromium.org/525353002/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:242: m_free =
reinterpret_cast<WrapperPersistentNode*>(free->m_raw);
On 2014/09/01 13:52:46, Mads Ager (chromium) wrote:
> Maybe wrap this in a method: free->nextFree().
> 
> Then we can have an assert in nextFree that the live bit is not set on the
> WrapperPersistentNode* that is returned?

I have added a takeSlot and freeSlot method in addition to the initSlot method I
described above.

https://codereview.chromium.org/525353002/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:249: return m_next->allocate();
On 2014/09/01 13:52:46, Mads Ager (chromium) wrote:
> Can we use iteration iteration from the outside instead of recursion? Maybe I
> should trust the compiler more to do tail-call optimization?

Done.

https://codereview.chromium.org/525353002/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:274: m_next->trace(visitor);
On 2014/09/01 13:52:46, Mads Ager (chromium) wrote:
> Can we use iteration from the outside instead of recursion?
> 
> for (WrapperPersistentRegion* current = ...; current; current =
current->next())
> {
>   current->trace();
> }
> 
> With a block size of 256 we can probably get a lot of blocks (at least on
> artificial benchmarks). Maybe I'm too old fashioned. I'm worried that the
> compiler will not perform tail call optimization here and that we have a risk
of
> blowing out the stack.

Done.

https://codereview.chromium.org/525353002/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:279: bool
removeIfNotLast(WrapperPersistentRegion** headPtr)
On 2014/09/02 05:22:05, haraken wrote:
> 
> Shall we move not-performance-sensitive methods to cpp files? It's not nice to
> increase Handle.h :)

Done.

https://codereview.chromium.org/525353002/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:285: if (m_prev) {
On 2014/09/02 05:22:05, haraken wrote:
> 
> Can we just call removeHead() here?

No, that is always removing the head entry via the threadState head pointer.

https://codereview.chromium.org/525353002/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:327: WrapperPersistentRegion* m_next;
On 2014/09/02 05:22:05, haraken wrote:
> 
> Shall we override 'operator new=' in a private section? If it's used, we'll
end
> up breaking the doubly-linked list unexpectedly.

Are you referring to the assignment operator? If so I have made both the
WrapperPersistentRegion and WrapperPersistentNode non-copyable.

https://codereview.chromium.org/525353002/diff/1/Source/platform/heap/ThreadS...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/525353002/diff/1/Source/platform/heap/ThreadS...
Source/platform/heap/ThreadState.cpp:335: while (m_wrapperPersistents) {
On 2014/09/01 13:52:46, Mads Ager (chromium) wrote:
> How about regions that are in the pool? Should we delete those here as well?

That seems like a good idea:) Fixed.

https://codereview.chromium.org/525353002/diff/1/Source/platform/heap/ThreadS...
Source/platform/heap/ThreadState.cpp:710: // Region was removed, ie. it is was
not the last region in the list.
On 2014/09/02 05:22:06, haraken wrote:
> 
> it is was => it was

Done.

https://codereview.chromium.org/525353002/diff/1/Source/platform/heap/ThreadS...
Source/platform/heap/ThreadState.cpp:711: if (m_wrapperPersistentRegionPoolSize
< 2) {
On 2014/09/02 05:22:06, haraken wrote:
> 
> What does the '< 2' check mean?

It means we will only keep at most two regions in the pool. This is to avoid
having too large a memory overhead.

https://codereview.chromium.org/525353002/diff/1/Source/platform/heap/ThreadS...
File Source/platform/heap/ThreadState.h (right):

https://codereview.chromium.org/525353002/diff/1/Source/platform/heap/ThreadS...
Source/platform/heap/ThreadState.h:664: WrapperPersistentRegion*
m_wrapperPersistents;
On 2014/09/02 05:22:06, haraken wrote:
> 
> m_wrapperPersistents => m_usedWrapperPersistents
> m_wrapperPersistentRegionPool => m_pooledWrapperPersistents
> 
> ?

Done. Changed them to

m_liveWrapperPersistents
m_pooledWrapperPersistents

https://codereview.chromium.org/525353002/diff/1/Source/platform/heap/ThreadS...
Source/platform/heap/ThreadState.h:665: WrapperPersistentRegion*
m_wrapperPersistentRegionPool;
On 2014/09/02 05:22:06, haraken wrote:
> 
> WrapperPersistentRegion is owned by either m_wrapperPersistents or
> m_wrapperPersistentRegionPool. Can we use
Deque<OwnPtr<WrapperPersistentRegion>>
> and avoid implementing insertHead, removeHead etc?

I would prefer to keep it a double-linked list to make internal removal fast.
Ie. there is a good chance for a region to be removed while in the middle of the
list via the removeIfNotLast method.

[Mads Ager (chromium), 2014-09-02 11:45:49]

LGTM, let's give this a spin on the perf bots and see how well this works. :)

https://codereview.chromium.org/525353002/diff/20001/Source/platform/heap/Han...
File Source/platform/heap/Handle.cpp (right):

https://codereview.chromium.org/525353002/diff/20001/Source/platform/heap/Han...
Source/platform/heap/Handle.cpp:2: * Copyright (C) 2014 Google Inc. All rights
reserved.
Let's use the short copyright header for new files?

[wibling-chromium, 2014-09-02 11:54:25]

Thanks for the review!

https://codereview.chromium.org/525353002/diff/20001/Source/platform/heap/Han...
File Source/platform/heap/Handle.cpp (right):

https://codereview.chromium.org/525353002/diff/20001/Source/platform/heap/Han...
Source/platform/heap/Handle.cpp:2: * Copyright (C) 2014 Google Inc. All rights
reserved.
On 2014/09/02 11:45:48, Mads Ager (chromium) wrote:
> Let's use the short copyright header for new files?

Done.

[haraken, 2014-09-02 13:00:37]

LGTM

(Though I still think that it would be better to just use Deque<OwnPtr> for the
chain of WrapperPersistentRegion and avoid implementing a lot of pointer logic.
The overhead of using the Deque<OwnPtr> won't matter since the chain won't be
updated so frequently.)

https://codereview.chromium.org/525353002/diff/40001/Source/platform/heap/Han...
File Source/platform/heap/Handle.cpp (right):

https://codereview.chromium.org/525353002/diff/40001/Source/platform/heap/Han...
Source/platform/heap/Handle.cpp:15: if (m_prev == 0 && m_next == 0)

if (!m_prev && !m_next)

https://codereview.chromium.org/525353002/diff/40001/Source/platform/heap/Han...
File Source/platform/heap/Handle.h (right):

https://codereview.chromium.org/525353002/diff/40001/Source/platform/heap/Han...
Source/platform/heap/Handle.h:190: m_regionOffset |=
wrapperPersistentLiveBitMask;

m_regionOffset = wrapperPersistentLiveBitMask;

?

https://codereview.chromium.org/525353002/diff/40001/Source/platform/heap/Han...
Source/platform/heap/Handle.h:318: void* WrapperPersistent<T>::operator
new(size_t size)

Drop |size|.

https://codereview.chromium.org/525353002/diff/40001/Source/platform/heap/Thr...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/525353002/diff/40001/Source/platform/heap/Thr...
Source/platform/heap/ThreadState.cpp:710: if
(!region->removeIfNotLast(&m_liveWrapperPersistents))

Can we avoid introducing removeIfNotLast and clean up the code by allocating an
anchor region?

https://codereview.chromium.org/525353002/diff/40001/Source/platform/heap/Thr...
Source/platform/heap/ThreadState.cpp:714: if
(m_numPooledWrapperPersistentRegions < 2) {

Add a comment about '< 2' or use a static constant variable with a descriptive
name instead of a hard-coded number.

Intuitively 2 seems a bit too small to me. If you use 2, won't it repeat
creating and deleting regions in create-element.html where a lot of wrappers are
created and deleted repeatedly?

https://codereview.chromium.org/525353002/diff/40001/Source/platform/heap/Thr...
File Source/platform/heap/ThreadState.h (right):

https://codereview.chromium.org/525353002/diff/40001/Source/platform/heap/Thr...
Source/platform/heap/ThreadState.h:666: size_t
m_numPooledWrapperPersistentRegions;

m_numPooledWrapperPersistentRegions => m_numberOfPooledWrapperPersistentRegions
or m_pooledWrapperPersistentRegionsCount

Blink prefers a fully-qualified variable name.

[wibling-chromium, 2014-09-02 13:55:33]

Thanks again for the review!

Regarding the Deque there can be quite a bit of regions allocated in some
benchmarks, such as bindings/create-element.html and AFAIK the Deque will not be
efficient when removing non-head elements.

https://codereview.chromium.org/525353002/diff/40001/Source/platform/heap/Han...
File Source/platform/heap/Handle.cpp (right):

https://codereview.chromium.org/525353002/diff/40001/Source/platform/heap/Han...
Source/platform/heap/Handle.cpp:15: if (m_prev == 0 && m_next == 0)
On 2014/09/02 13:00:37, haraken wrote:
> 
> if (!m_prev && !m_next)

Done.

https://codereview.chromium.org/525353002/diff/40001/Source/platform/heap/Han...
File Source/platform/heap/Handle.h (right):

https://codereview.chromium.org/525353002/diff/40001/Source/platform/heap/Han...
Source/platform/heap/Handle.h:190: m_regionOffset |=
wrapperPersistentLiveBitMask;
On 2014/09/02 13:00:37, haraken wrote:
> 
> m_regionOffset = wrapperPersistentLiveBitMask;
> 
> ?

No, since the offset to the base is already in the field and that should be kept
around. We are overloading the field to have a live bit in the lowest bit,
similar to the HeapObjectHeader's size.

https://codereview.chromium.org/525353002/diff/40001/Source/platform/heap/Han...
Source/platform/heap/Handle.h:318: void* WrapperPersistent<T>::operator
new(size_t size)
On 2014/09/02 13:00:37, haraken wrote:
> 
> Drop |size|.

I added an assert(size == sizeof(WrapperPersistent<T>) instead since it seems
like a good thing to check.

https://codereview.chromium.org/525353002/diff/40001/Source/platform/heap/Thr...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/525353002/diff/40001/Source/platform/heap/Thr...
Source/platform/heap/ThreadState.cpp:710: if
(!region->removeIfNotLast(&m_liveWrapperPersistents))
On 2014/09/02 13:00:37, haraken wrote:
> 
> Can we avoid introducing removeIfNotLast and clean up the code by allocating
an
> anchor region?

No, I did try having an anchor and it doesn't simplify things. For
removeIfNotLast it wouldn't help since the entry could be anywhere in the list
(and likely is in the middle).

https://codereview.chromium.org/525353002/diff/40001/Source/platform/heap/Thr...
Source/platform/heap/ThreadState.cpp:714: if
(m_numPooledWrapperPersistentRegions < 2) {
On 2014/09/02 13:00:37, haraken wrote:
> 
> Add a comment about '< 2' or use a static constant variable with a descriptive
> name instead of a hard-coded number.

Added a constant.

> Intuitively 2 seems a bit too small to me. If you use 2, won't it repeat
> creating and deleting regions in create-element.html where a lot of wrappers
are
> created and deleted repeatedly?

Yes, 2 does seem a bit small. OTOH in most real world scenarios we won't have a
lot of wrappers so keeping a lot of memory allocated and unused seems wasteful. 
I am contemplating making region size adaptable, such that a region could start
of at 256 and grow if there is a lot of wrapper allocations. 
In that case two regions could be quite a bit of space.
From tests I have seen some improvements when the region size is 16384 instead
of 256, but then the wasted memory overhead if unused is quite large (~400K).

I will continue to investigate this after landing this patch to see if there is
additional perf gains without too much memory overhead.

https://codereview.chromium.org/525353002/diff/40001/Source/platform/heap/Thr...
File Source/platform/heap/ThreadState.h (right):

https://codereview.chromium.org/525353002/diff/40001/Source/platform/heap/Thr...
Source/platform/heap/ThreadState.h:666: size_t
m_numPooledWrapperPersistentRegions;
On 2014/09/02 13:00:37, haraken wrote:
> 
> m_numPooledWrapperPersistentRegions =>
m_numberOfPooledWrapperPersistentRegions
> or m_pooledWrapperPersistentRegionsCount
> 
> Blink prefers a fully-qualified variable name.

Done.

[haraken, 2014-09-02 14:53:51]

Just nits. Still LGTM.

https://codereview.chromium.org/525353002/diff/60001/Source/platform/heap/Han...
File Source/platform/heap/Handle.h (right):

https://codereview.chromium.org/525353002/diff/60001/Source/platform/heap/Han...
Source/platform/heap/Handle.h:189: ASSERT(m_regionOffset && !isAlive());

It's better to have two separate ASSERTs to see which condition failed.

ASSERT(m_regionOffset);
ASSERT(!isAlive());

https://codereview.chromium.org/525353002/diff/60001/Source/platform/heap/Han...
Source/platform/heap/Handle.h:244: virtual void trace(Visitor* visitor) {
visitor->mark(static_cast<T*>(m_raw)); }

Add ASSERT(isAlive());

https://codereview.chromium.org/525353002/diff/60001/Source/platform/heap/Han...
Source/platform/heap/Handle.h:268: if (!m_freeHead)

Add ASSERT(m_count == wrapperPersistentsPerRegion);

https://codereview.chromium.org/525353002/diff/60001/Source/platform/heap/Han...
Source/platform/heap/Handle.h:274: m_count++;

Add ASSERT(m_count < wrapperPersistentsPerRegion);

https://codereview.chromium.org/525353002/diff/60001/Source/platform/heap/Han...
Source/platform/heap/Handle.h:283: m_count--;

Add ASSERT(m_count > 0);

https://codereview.chromium.org/525353002/diff/60001/Source/platform/heap/Han...
Source/platform/heap/Handle.h:302: for (int i = 0; i <
wrapperPersistentsPerRegion && live < m_count; ++i) {

In Debug builds, it might be worth fully scanning the list and verify that the
number of alive entries are equal to m_count.

https://codereview.chromium.org/525353002/diff/60001/Source/platform/heap/Thr...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/525353002/diff/60001/Source/platform/heap/Thr...
Source/platform/heap/ThreadState.cpp:697: WrapperPersistentRegion*
ThreadState::addWrapperPersistentRegion()

takeWrapperPersistentRegion & freeWrapperPersistentRegion sounds better to be
consistent with takeSlot & freeSlot.

[wibling-chromium, 2014-09-03 07:51:42]

Thanks for the review and the help with the component build! I have added a
PLATFORM_EXPORT and reproduced without it.

Replies inline below to the specific comments.

https://codereview.chromium.org/525353002/diff/60001/Source/platform/heap/Han...
File Source/platform/heap/Handle.h (right):

https://codereview.chromium.org/525353002/diff/60001/Source/platform/heap/Han...
Source/platform/heap/Handle.h:189: ASSERT(m_regionOffset && !isAlive());
On 2014/09/02 14:53:51, haraken wrote:
> 
> It's better to have two separate ASSERTs to see which condition failed.
> 
> ASSERT(m_regionOffset);
> ASSERT(!isAlive());

Done.

https://codereview.chromium.org/525353002/diff/60001/Source/platform/heap/Han...
Source/platform/heap/Handle.h:244: virtual void trace(Visitor* visitor) {
visitor->mark(static_cast<T*>(m_raw)); }
On 2014/09/02 14:53:51, haraken wrote:
> 
> Add ASSERT(isAlive());

Done.

https://codereview.chromium.org/525353002/diff/60001/Source/platform/heap/Han...
Source/platform/heap/Handle.h:268: if (!m_freeHead)
On 2014/09/02 14:53:50, haraken wrote:
> 
> Add ASSERT(m_count == wrapperPersistentsPerRegion);

Done.

https://codereview.chromium.org/525353002/diff/60001/Source/platform/heap/Han...
Source/platform/heap/Handle.h:274: m_count++;
On 2014/09/02 14:53:51, haraken wrote:
> 
> Add ASSERT(m_count < wrapperPersistentsPerRegion);

Done.

https://codereview.chromium.org/525353002/diff/60001/Source/platform/heap/Han...
Source/platform/heap/Handle.h:283: m_count--;
On 2014/09/02 14:53:51, haraken wrote:
> 
> Add ASSERT(m_count > 0);

Done.

https://codereview.chromium.org/525353002/diff/60001/Source/platform/heap/Han...
Source/platform/heap/Handle.h:302: for (int i = 0; i <
wrapperPersistentsPerRegion && live < m_count; ++i) {
On 2014/09/02 14:53:51, haraken wrote:
> 
> In Debug builds, it might be worth fully scanning the list and verify that the
> number of alive entries are equal to m_count.

Done.

https://codereview.chromium.org/525353002/diff/60001/Source/platform/heap/Thr...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/525353002/diff/60001/Source/platform/heap/Thr...
Source/platform/heap/ThreadState.cpp:697: WrapperPersistentRegion*
ThreadState::addWrapperPersistentRegion()
On 2014/09/02 14:53:51, haraken wrote:
> 
> takeWrapperPersistentRegion & freeWrapperPersistentRegion sounds better to be
> consistent with takeSlot & freeSlot.

Done.

[wibling-chromium, 2014-09-03 07:52:04]

The CQ bit was checked by wibling@chromium.org

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

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

[zerny-chromium, 2014-09-03 08:06:46]

Was much easier to read second time through! lgtm.

[commit-bot: I haz the power, 2014-09-03 09:07:21]

Committed patchset #6 (id:100001) as 181298

[Fabrice (no longer in Chrome), 2014-09-05 18:37:31]

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

[Fabrice (no longer in Chrome), 2014-09-05 18:37:32]

Please see the comments below.
From reading the comments and the previous patch sets, I understand what you
were trying to do but you cannot reuse space in this fashion in C++ unless you
use placement new. And even then, you cannot rely on the previous state of the
state.
Per the C++ standard, the compiler has total freedom over what it does and
initializes (or not) in the space before the constructor or after the destructor
are executed.

As it is, you are relying on undefined behavior which breaks with GCC 4.9.
Therefore, I suggest a revert.

https://codereview.chromium.org/525353002/diff/100001/Source/platform/heap/Ha...
File Source/platform/heap/Handle.h (right):

https://codereview.chromium.org/525353002/diff/100001/Source/platform/heap/Ha...
Source/platform/heap/Handle.h:171: m_regionOffset &=
~wrapperPersistentLiveBitMask;
From what I understand, isAlive returns false before the constructor is called
and after the destructor is called, right?

https://codereview.chromium.org/525353002/diff/100001/Source/platform/heap/Ha...
Source/platform/heap/Handle.h:183: // pointer should be 0.
I take it from the above comment you are initializing some stuff on the memory
occupied by the object before the constructor is called?
I think this is undefined behavior.

https://codereview.chromium.org/525353002/diff/100001/Source/platform/heap/Ha...
Source/platform/heap/Handle.h:289: ASSERT(!object->isAlive());
Per the above comment, you are calling this after the destructor of object has
been called?
This is undefined behavior per the C++ standard

[wibling-chromium, 2014-09-08 09:38:23]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/553483004/ by wibling@chromium.org.

The reason for reverting is: Reverting due to browser test failures..