Trace ScriptWrappableVisitor.m_markingDeque by oilpan gc

Trace ScriptWrappableVisitor.m_markingDeque by oilpan gc

This cl has two parts:

1. Make ScriptWrappableVisitor.m_markingDeque part of the root set for the
   oilpan gc so wrapper marked objects will not be collected.

Without this cl, v8 can detect wrapper during incremental marking, but the v8
scavenger is free to collect the wrapper. Subsequent oilpan gc will collect
ScriptWrappable and recorded internal fields of v8 wrapper are no longer valid.
With this cl, oilpan will not collect ScriptWrappables if they are in the
marking deque. Other slightly more complicated approach would be to cleanup the
marking deque after oilpan gc and remove dead objects.

2. Add freshly associated wrappers to the ScriptWrappableVisitor.m_markingDeque
   to ensure they will be processed when black allocation in v8 is on.

When black allcation is on, v8 creates new objects black, and completely skips
any processing, including wrapper detection. Such object would be undetected and
not traced at all. Because of that, on all wrapper associations (which
happen shortly after creation) we put objects into the marking deque. This way
we make sure the object will be traced.

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

[Marcel Hlopko, 2016-06-07 13:39:13]

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

[Marcel Hlopko, 2016-06-07 13:39:19]

Ptal.

[haraken, 2016-06-08 01:10:12]

> Make ScriptWrappableVisitor.m_markingDeque part of the root set for the
>   oilpan gc so wrapper marked objects will not be collected.

This approach will work but I'm not sure if it's a good design. It looks a bit
strange to ask Oilpan to protect the objects while V8 is doing incremental
marking. Wouldn't it make more sense to make V8 protect the wrappers while V8 is
doing incremental marking (i.e., make V8's minor GC not collect wrappers in the
marking deque)?

[Marcel Hlopko, 2016-06-08 06:45:06]

I tried multiple solutions in the v8, but none which would not regress
performance on our benchmarks, or be extremely painful to maintain. And at the
end, when V8 protects its wrappers, oilpan ScriptWrappables will be kept alive
too. For the oilpan side, there is no difference in what survives, we just free
v8 from having to do the useless work. Some complexity arises from the fact that
v8 object move, that's why we get internal fields only.

As I look into the code I register wrappers also when we are not marking, which
is stupid and I'll fix it. Another thing we can do is not to keep all wrappers
alive, but to cleanup the deque after gc so it doesn't contain garbage. Wdyt?

[haraken, 2016-06-08 08:56:08]

> Without this cl, v8 can detect wrapper during incremental marking, but the v8
> scavenger is free to collect the wrapper. Subsequent oilpan gc will collect
> ScriptWrappable and recorded internal fields of v8 wrapper are no longer
valid.
> With this cl, oilpan will not collect ScriptWrappables if they are in the
> marking deque. Other slightly more complicated approach would be to cleanup
the
> marking deque after oilpan gc and remove dead objects.

Hmm, I thought about this a bit more and begin to think that it would be better
to the latter approach you mentioned above.

If we take the approach in this CL, Oilpan will end up with keeping alive
objects that are actually dead but happen to be in the deque of incremental
marking. It will delay collecting Oilpan objects -- according to our experience,
this is a bad thing because Oilpan's GCs are not triggered as frequently as V8's
GCs (also Oilpan objects can retain a lot of memory behind it).

Would it be possible to iterate the marking deque at the end of Oilpan's GC
(i.e., at the end of ThreadHeap::collectGarbage) and clear out dead
ScriptWrappables in the deque? You don't need to actually remove the pointers
from the deque. I think you just replace the pointer with nullptr.

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

https://codereview.chromium.org/2043033002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp:149: void
ScriptWrappableVisitor::traceWrappersMarkingDequeTracing(v8::Isolate* isolate,
Visitor* visitor)

traceWrappersInMarkingDeque

https://codereview.chromium.org/2043033002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.h (right):

https://codereview.chromium.org/2043033002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.h:76: auto
perIsolateData = V8PerIsolateData::from(isolate);

Add a comment about what this is doing.

https://codereview.chromium.org/2043033002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.h:76: auto
perIsolateData = V8PerIsolateData::from(isolate);

I guess this will regress performance of wrapper creation. But I'm okay with
worrying about the regression later.

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

https://codereview.chromium.org/2043033002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/ThreadState.h:430: // Make sure the
isolate set by registerTraceDOMWrapers is the same as

Or can we merge the two register functions?

[Marcel Hlopko, 2016-06-08 14:17:08]

Addressed all the points. Marking deque is now cleaned after oilpan gc. Wdyt?

[haraken, 2016-06-08 14:38:23]

The approach looks good.

https://codereview.chromium.org/2043033002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ActiveScriptWrappable.cpp
(right):

https://codereview.chromium.org/2043033002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ActiveScriptWrappable.cpp:37:
visitor->RegisterV8Reference(std::make_pair(wrapperTypeInfo, scriptWrappable));

Would you help me understand why you need to push ActiveScriptWrappables to the
marking deque?

I'm assuming that traceActiveScriptWrappables should be called multiple times in
the incremental marking phase because the pending activity may change over time.

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

https://codereview.chromium.org/2043033002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp:144: void
ScriptWrappableVisitor::cleanMarkingDeque()

cleanMarkingDeque => invalidateDeadObjectsInMarkingDeque

https://codereview.chromium.org/2043033002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp:150:
isDead()) {

I think this check should be:

  if (!...->isMarked())

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

https://codereview.chromium.org/2043033002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/Heap.cpp:608: 

You need to clean the marking deque here as well.

[Marcel Hlopko, 2016-06-08 15:12:48]

https://codereview.chromium.org/2043033002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ActiveScriptWrappable.cpp
(right):

https://codereview.chromium.org/2043033002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ActiveScriptWrappable.cpp:37:
visitor->RegisterV8Reference(std::make_pair(wrapperTypeInfo, scriptWrappable));
On 2016/06/08 at 14:38:23, haraken wrote:
> Would you help me understand why you need to push ActiveScriptWrappables to
the marking deque?
> 
> I'm assuming that traceActiveScriptWrappables should be called multiple times
in the incremental marking phase because the pending activity may change over
time.

Here it's just for the consistency. Objects external to ScriptWrappableVisitor
can only add to the deque, cannot initiate tracing. Tracing is initiated only by
AdvanceTracing.

However you are correct that traceActiveScriptWrappables will be called multiple
times. Once during TracePrologue, as an overapproximation, and then at the
"atomic pause of wrapper tracing".

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

https://codereview.chromium.org/2043033002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp:144: void
ScriptWrappableVisitor::cleanMarkingDeque()
On 2016/06/08 at 14:38:23, haraken wrote:
> cleanMarkingDeque => invalidateDeadObjectsInMarkingDeque

Done.

https://codereview.chromium.org/2043033002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp:150:
isDead()) {
Done.

[haraken, 2016-06-09 06:28:31]

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

https://codereview.chromium.org/2043033002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/Heap.cpp:608: 
On 2016/06/08 14:38:23, haraken wrote:
> 
> You need to clean the marking deque here as well.

Address this comment.

https://codereview.chromium.org/2043033002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp
(right):

https://codereview.chromium.org/2043033002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp:44:
return;

Shall we add DCHECK(m_markingDeque.isEmpty())?

https://codereview.chromium.org/2043033002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp:145: for
(auto it = m_markingDeque.begin(); it != m_markingDeque.end(); ++it) {

Just to confirm: m_markingDeque contains only ScriptWrappables created on this
thread, right? If it's not guaranteed, it will break assumptions in Oilpan's
per-thread heap.

https://codereview.chromium.org/2043033002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/2043033002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/Heap.cpp:570:
state->invalidateDeadObjectsInWrappersMarkingDeque();

I begin to think it is not enough to call
invalidateDeadObjectsInWrappersMarkingDeque from the GCing thread. We need to
call it for all other threads that get involved in the GC.

Specifically, you need to move the call to ThreadState::postGC() so that it's
called for all ThreadStates that get involved in the GC.

[Marcel Hlopko, 2016-06-09 09:00:50]

https://codereview.chromium.org/2043033002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp
(right):

https://codereview.chromium.org/2043033002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp:44:
return;
On 2016/06/09 at 06:28:31, haraken wrote:
> Shall we add DCHECK(m_markingDeque.isEmpty())?

As far as I meant it, no. This method will be called multiple times, everytime
v8 discovers new wrappers, and everytime blink associates new wrappers. Calls to
AdvanceTracing happen independently to RegisterV8Reference.

https://codereview.chromium.org/2043033002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp:145: for
(auto it = m_markingDeque.begin(); it != m_markingDeque.end(); ++it) {
On 2016/06/09 at 06:28:31, haraken wrote:
> Just to confirm: m_markingDeque contains only ScriptWrappables created on this
thread, right? If it's not guaranteed, it will break assumptions in Oilpan's
per-thread heap.

Yes, as long as an isolate corresponds to a thread.

https://codereview.chromium.org/2043033002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/2043033002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/Heap.cpp:570:
state->invalidateDeadObjectsInWrappersMarkingDeque();
On 2016/06/09 at 06:28:31, haraken wrote:
> I begin to think it is not enough to call
invalidateDeadObjectsInWrappersMarkingDeque from the GCing thread. We need to
call it for all other threads that get involved in the GC.
> 
> Specifically, you need to move the call to ThreadState::postGC() so that it's
called for all ThreadStates that get involved in the GC.

You are right. Done.

[haraken, 2016-06-09 11:29:03]

Mostly looks good :) Here is a final round of comments.

https://codereview.chromium.org/2043033002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp
(right):

https://codereview.chromium.org/2043033002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp:44:
return;
On 2016/06/09 09:00:50, Marcel Hlopko wrote:
> On 2016/06/09 at 06:28:31, haraken wrote:
> > Shall we add DCHECK(m_markingDeque.isEmpty())?
> 
> As far as I meant it, no. This method will be called multiple times, everytime
> v8 discovers new wrappers, and everytime blink associates new wrappers. Calls
to
> AdvanceTracing happen independently to RegisterV8Reference.

Ah, makes sense.

https://codereview.chromium.org/2043033002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp
(right):

https://codereview.chromium.org/2043033002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp:43: if
(!m_tracingInProgress) {

Just help me understand: When do we hit this branch?

https://codereview.chromium.org/2043033002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp:59:
m_markingDeque.append(scriptWrappable);

Won't this add ScriptWrappables regardless of whether V8 is doing incremental
marking or not? i.e., do we really need to add ScriptWrappables when V8 is not
running incremental tracing?

In the current implementation, I think m_markingDeque will end up with holding
almost all ScriptWrappables (which may be created when V8 doesn't yet start the
incremental marking).

[Marcel Hlopko, 2016-06-09 11:56:54]

I also moved tracing of active script wrappables to the epilogue.

https://codereview.chromium.org/2043033002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp
(right):

https://codereview.chromium.org/2043033002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp:43: if
(!m_tracingInProgress) {
On 2016/06/09 at 11:29:03, haraken wrote:
> Just help me understand: When do we hit this branch?

When we associate wrapper with script wrappable, but we are not tracing. Maybe
the naming is confusing, as the meaning changed during my work. Initially it
meant V8 enterered atomic pause, gave us wrappers, and we are tracing. Now (not
landed in v8 yet :) it means V8 started incremental marking, and is giving us
wrappers as it finds them, and when it'll be in the mood, it will call
AdvanceTracing.

https://codereview.chromium.org/2043033002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp:59:
m_markingDeque.append(scriptWrappable);
On 2016/06/09 at 11:29:03, haraken wrote:
> Won't this add ScriptWrappables regardless of whether V8 is doing incremental
marking or not? i.e., do we really need to add ScriptWrappables when V8 is not
running incremental tracing?
> 
> In the current implementation, I think m_markingDeque will end up with holding
almost all ScriptWrappables (which may be created when V8 doesn't yet start the
incremental marking).

Is the meaning cleaner with my reply to the previous comment?

[haraken, 2016-06-09 14:09:10]

LGTM with a question.

https://codereview.chromium.org/2043033002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp
(right):

https://codereview.chromium.org/2043033002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp:43: if
(!m_tracingInProgress) {
On 2016/06/09 11:56:54, Marcel Hlopko wrote:
> On 2016/06/09 at 11:29:03, haraken wrote:
> > Just help me understand: When do we hit this branch?
> 
> When we associate wrapper with script wrappable, but we are not tracing. Maybe
> the naming is confusing, as the meaning changed during my work. Initially it
> meant V8 enterered atomic pause, gave us wrappers, and we are tracing. Now
(not
> landed in v8 yet :) it means V8 started incremental marking, and is giving us
> wrappers as it finds them, and when it'll be in the mood, it will call
> AdvanceTracing.

Given the following timeline:

[Mutator] => [Incremental marking part 1] => [Mutator] => [Incremental marking
part 2] => [Mutator] => [Atomic pause] => [Mutator]

m_tracingInProgress is true between [Incremental marking part 1] and [Atomic
pause], right?

[Marcel Hlopko, 2016-06-09 14:10:41]

> 
> Given the following timeline:
> 
> [Mutator] => [Incremental marking part 1] => [Mutator] => [Incremental marking
part 2] => [Mutator] => [Atomic pause] => [Mutator]
> 
> m_tracingInProgress is true between [Incremental marking part 1] and [Atomic
pause], right?

Yes.

[Marcel Hlopko, 2016-06-09 14:10:49]

The CQ bit was checked by hlopko@chromium.org

[commit-bot: I haz the power, 2016-06-09 14:11:06]

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

[commit-bot: I haz the power, 2016-06-09 14:32:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-09 14:32:44]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Marcel Hlopko, 2016-06-09 14:56:47]

The CQ bit was checked by hlopko@chromium.org

[Marcel Hlopko, 2016-06-09 14:56:47]

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/2043033002/#ps200001
(title: "Fix callers of registerTraceDOMWrappers")

[commit-bot: I haz the power, 2016-06-09 14:57:17]

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

[commit-bot: I haz the power, 2016-06-09 18:50:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-09 18:50:38]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Marcel Hlopko, 2016-06-10 07:32:01]

The CQ bit was checked by hlopko@chromium.org

[commit-bot: I haz the power, 2016-06-10 07:32:10]

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

[commit-bot: I haz the power, 2016-06-10 09:23:03]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-06-10 09:23:09]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-10 09:24:06]

Description was changed from

==========
Trace ScriptWrappableVisitor.m_markingDeque by oilpan gc

This cl has two parts:

1. Make ScriptWrappableVisitor.m_markingDeque part of the root set for the
   oilpan gc so wrapper marked objects will not be collected.

Without this cl, v8 can detect wrapper during incremental marking, but the v8
scavenger is free to collect the wrapper. Subsequent oilpan gc will collect
ScriptWrappable and recorded internal fields of v8 wrapper are no longer valid.
With this cl, oilpan will not collect ScriptWrappables if they are in the
marking deque. Other slightly more complicated approach would be to cleanup the
marking deque after oilpan gc and remove dead objects.

2. Add freshly associated wrappers to the ScriptWrappableVisitor.m_markingDeque
   to ensure they will be processed when black allocation in v8 is on.

When black allcation is on, v8 creates new objects black, and completely skips
any processing, including wrapper detection. Such object would be undetected and
not traced at all. Because of that, on all wrapper associations (which
happen shortly after creation) we put objects into the marking deque. This way
we make sure the object will be traced.

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

to

==========
Trace ScriptWrappableVisitor.m_markingDeque by oilpan gc

This cl has two parts:

1. Make ScriptWrappableVisitor.m_markingDeque part of the root set for the
   oilpan gc so wrapper marked objects will not be collected.

Without this cl, v8 can detect wrapper during incremental marking, but the v8
scavenger is free to collect the wrapper. Subsequent oilpan gc will collect
ScriptWrappable and recorded internal fields of v8 wrapper are no longer valid.
With this cl, oilpan will not collect ScriptWrappables if they are in the
marking deque. Other slightly more complicated approach would be to cleanup the
marking deque after oilpan gc and remove dead objects.

2. Add freshly associated wrappers to the ScriptWrappableVisitor.m_markingDeque
   to ensure they will be processed when black allocation in v8 is on.

When black allcation is on, v8 creates new objects black, and completely skips
any processing, including wrapper detection. Such object would be undetected and
not traced at all. Because of that, on all wrapper associations (which
happen shortly after creation) we put objects into the marking deque. This way
we make sure the object will be traced.

LOG=no
BUG=468240

Committed: https://crrev.com/b2bf69081a17b107dc4ec9c2cc5dd64d961c7e0d
Cr-Commit-Position: refs/heads/master@{#399141}
==========

[commit-bot: I haz the power, 2016-06-10 09:24:07]

Patchset 11 (id:??) landed as
https://crrev.com/b2bf69081a17b107dc4ec9c2cc5dd64d961c7e0d
Cr-Commit-Position: refs/heads/master@{#399141}