Store raw pointers in ScriptWrappableVisitor markind deque

Store raw pointers in ScriptWrappableVisitor markind deque

With a few experiments making wrapper tracing incremental we realized we need to
introduce write barriers anyway, and having script wrappables in the marking
deque would make write barriers in non-script-wrappable classes complicated.
This cl changes the marking deque to hold generic raw pointers + needed function
pointers.

We need one function to do the marking and adding members to the deque, and we
need one function for getting the header, for correct invalidation of headers
when oilpan gc runs during wrapper tracing. During invalidation, we also have to
remove headers of deleted objects from m_headersToUnmark.

We also had to revert to the previous implementation, where there is
WrapperTypeInfo.traceWrappers instead of WraperTypeInfo.getHeapObjectHeader.
Before we marked the object, and then added it to the marking deque for
expansion. Now we add object to the marking deque, and marking and expansion is
done in the one incremental step.

LOG=no
BUG=468240
Committed: https://crrev.com/8111a7789baafc20cd90808caf30fe5041a361ca
Cr-Commit-Position: refs/heads/master@{#400722}

[Marcel Hlopko, 2016-06-20 10:51:04]

hlopko@chromium.org changed reviewers:
+ haraken@chromium.org, hpayer@chromium.org, jochen@chromium.org

[Marcel Hlopko, 2016-06-20 10:51:18]

Ptal.

[haraken, 2016-06-20 11:10:38]

https://codereview.chromium.org/2084513002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/2084513002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/Heap.cpp:575:
state->invalidateDeadObjectsInWrappersMarkingDeque();

Why do you need this? Don't you already invalidate dead objects in the following
postGC()?

https://codereview.chromium.org/2084513002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/heap/WrapperVisitor.h (right):

https://codereview.chromium.org/2084513002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/WrapperVisitor.h:100:
pushToMarkingDeque(

Won't this regress performance?

[Marcel Hlopko, 2016-06-20 11:29:13]

Incorporated comments.

https://codereview.chromium.org/2084513002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/2084513002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/Heap.cpp:575:
state->invalidateDeadObjectsInWrappersMarkingDeque();
On 2016/06/20 at 11:10:38, haraken wrote:
> Why do you need this? Don't you already invalidate dead objects in the
following postGC()?

You're right, this is just a result of bad rebase.

https://codereview.chromium.org/2084513002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/heap/WrapperVisitor.h (right):

https://codereview.chromium.org/2084513002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/WrapperVisitor.h:100:
pushToMarkingDeque(
On 2016/06/20 at 11:10:38, haraken wrote:
> Won't this regress performance?

Good point! Added check for wrapperHeaderMark. I could mark object on push, and
expand during 
advancing marking This way objects are marked sooner and duplicated entries to
the marking 
deque are pushed. I just feel [ push, ..., pop, mark, expand ] is what people
expect, instead of 
[ mark, push, ..., pop, expand ]. Wdyt?

[haraken, 2016-06-20 14:20:57]

LGTM

https://codereview.chromium.org/2084513002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp
(right):

https://codereview.chromium.org/2084513002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp:53:
m_headersToUnmark.clear();

Not related to this CL, don't we need to clear m_markingDeque here?

https://codereview.chromium.org/2084513002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h
(right):

https://codereview.chromium.org/2084513002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h:38: 

Unnecessary empty line.

https://codereview.chromium.org/2084513002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h:46: inline
bool shouldBeInvalidated()

I'd make shouldBeInvalidated() and invalidate() private and add a friend class
for ScriptWrappableVisitor, since it's dangerous that these methods are called
outside ScriptWrappableVisitor::invalidateDeadObjectsInMarkingDeque().

https://codereview.chromium.org/2084513002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/2084513002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/Heap.cpp:35: #include
"platform/RuntimeEnabledFeatures.h"

Drop this change.

https://codereview.chromium.org/2084513002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/heap/ThreadState.h (right):

https://codereview.chromium.org/2084513002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/ThreadState.h:531: void
invalidateDeadObjectsInWrappersMarkingDeque()

Is this method used somewhere?

[Marcel Hlopko, 2016-06-20 16:26:46]

https://codereview.chromium.org/2084513002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp
(right):

https://codereview.chromium.org/2084513002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp:53:
m_headersToUnmark.clear();
On 2016/06/20 at 14:20:57, haraken wrote:
> Not related to this CL, don't we need to clear m_markingDeque here?

You are right, for abort case, we need to clear the deque. For TraceEpilogue,
deque should be empty. Adding dcheck for epilogue, clear for abort.

https://codereview.chromium.org/2084513002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h
(right):

https://codereview.chromium.org/2084513002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h:38: 
On 2016/06/20 at 14:20:57, haraken wrote:
> Unnecessary empty line.

Removed.

https://codereview.chromium.org/2084513002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h:46: inline
bool shouldBeInvalidated()
On 2016/06/20 at 14:20:57, haraken wrote:
> I'd make shouldBeInvalidated() and invalidate() private and add a friend class
for ScriptWrappableVisitor, since it's dangerous that these methods are called
outside ScriptWrappableVisitor::invalidateDeadObjectsInMarkingDeque().

Done.

https://codereview.chromium.org/2084513002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/2084513002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/Heap.cpp:35: #include
"platform/RuntimeEnabledFeatures.h"
On 2016/06/20 at 14:20:57, haraken wrote:
> Drop this change.

Done.

https://codereview.chromium.org/2084513002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/heap/ThreadState.h (right):

https://codereview.chromium.org/2084513002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/ThreadState.h:531: void
invalidateDeadObjectsInWrappersMarkingDeque()
On 2016/06/20 at 14:20:57, haraken wrote:
> Is this method used somewhere?

Removed.

[Marcel Hlopko, 2016-06-20 16:26:53]

The CQ bit was checked by hlopko@chromium.org

[Marcel Hlopko, 2016-06-20 16:26:53]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2084513002/#ps80001
(title: "Cleanup")

[commit-bot: I haz the power, 2016-06-20 16:27:09]

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

[commit-bot: I haz the power, 2016-06-20 18:05:19]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-06-20 18:10:44]

Description was changed from

==========
Store raw pointers in ScriptWrappableVisitor markind deque

With a few experiments making wrapper tracing incremental we realized we need to
introduce write barriers anyway, and having script wrappables in the marking
deque would make write barriers in non-script-wrappable classes complicated.
This cl changes the marking deque to hold generic raw pointers + needed function
pointers.

We need one function to do the marking and adding members to the deque, and we
need one function for getting the header, for correct invalidation of headers
when oilpan gc runs during wrapper tracing. During invalidation, we also have to
remove headers of deleted objects from m_headersToUnmark.

We also had to revert to the previous implementation, where there is
WrapperTypeInfo.traceWrappers instead of WraperTypeInfo.getHeapObjectHeader.
Before we marked the object, and then added it to the marking deque for
expansion. Now we add object to the marking deque, and marking and expansion is
done in the one incremental step.

LOG=no
BUG=468240
==========

to

==========
Store raw pointers in ScriptWrappableVisitor markind deque

With a few experiments making wrapper tracing incremental we realized we need to
introduce write barriers anyway, and having script wrappables in the marking
deque would make write barriers in non-script-wrappable classes complicated.
This cl changes the marking deque to hold generic raw pointers + needed function
pointers.

We need one function to do the marking and adding members to the deque, and we
need one function for getting the header, for correct invalidation of headers
when oilpan gc runs during wrapper tracing. During invalidation, we also have to
remove headers of deleted objects from m_headersToUnmark.

We also had to revert to the previous implementation, where there is
WrapperTypeInfo.traceWrappers instead of WraperTypeInfo.getHeapObjectHeader.
Before we marked the object, and then added it to the marking deque for
expansion. Now we add object to the marking deque, and marking and expansion is
done in the one incremental step.

LOG=no
BUG=468240

Committed: https://crrev.com/8111a7789baafc20cd90808caf30fe5041a361ca
Cr-Commit-Position: refs/heads/master@{#400722}
==========

[commit-bot: I haz the power, 2016-06-20 18:10:46]

Patchset 5 (id:??) landed as
https://crrev.com/8111a7789baafc20cd90808caf30fe5041a361ca
Cr-Commit-Position: refs/heads/master@{#400722}