Use EmbedderHeapTracer instead of object grouping when trace_embedder_heap flag is set

Use EmbedderHeapTracer instead of object grouping when embedder sets the heap tracer.

When the embedder sets the heap tracer, V8, during marking, will collect all reachable wrappers, and then ask embedder to trace its heap. The embedder is expected to call PersistentBase::RegisterExternalReference with all wrappers reachable from the given ones. This fixed point iteration happens in MarkCompact::ProcessEphemeralMarking.

For more efficient object visiting during marking, we need a special JS_API_OBJECT_TYPE (in tandem with already existing JS_SPECIAL_API_OBJECT_TYPE) and corresponding visitor (JSApiObjectVisitor).

BUG=chromium:468240
LOG=no

Committed: https://crrev.com/6d1f7282aff7cf332bbecf3f63e99b2e7716fa14
Cr-Commit-Position: refs/heads/master@{#35412}

[Marcel Hlopko, 2016-03-31 15:59:26]

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

[Marcel Hlopko, 2016-03-31 16:08:54]

Hello Gentlemen,

Ptal.

I'm not very happy with the ProcessEphemeralMarking method. I tried to extract
weak collection processing to a separate method, which looked cleaner, and worth
having even as it duplicates method structure (while (work_to_do) { ...
ProcessMarkingDeque(); }). I would rename methods unrelated to this cl, so I
decided I'll upload the refactoring as a separate cl (which can be rejected if
unnecessary :)

[jochen (gone - plz use gerrit), 2016-04-01 08:44:46]

Description was changed from

==========
Use EmbedderHeapTracer instead of object grouping when trace_embedder_heap flag
is set

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

to

==========
Use EmbedderHeapTracer instead of object grouping when trace_embedder_heap flag
is set

BUG=chromium:468240
LOG=no
==========

[jochen (gone - plz use gerrit), 2016-04-01 08:49:12]

https://codereview.chromium.org/1844413002/diff/1/src/heap/incremental-markin...
File src/heap/incremental-marking.cc (right):

https://codereview.chromium.org/1844413002/diff/1/src/heap/incremental-markin...
src/heap/incremental-marking.cc:738: if
(!heap_->mark_compact_collector()->UsingEmbedderHeapTracer()) {
in principle, we could use both at the same time, no?

https://codereview.chromium.org/1844413002/diff/1/src/heap/mark-compact.cc
File src/heap/mark-compact.cc (right):

https://codereview.chromium.org/1844413002/diff/1/src/heap/mark-compact.cc#ne...
src/heap/mark-compact.cc:1923: js_object->WasConstructedFromApiFunction()) {
eventually, we should add a flag to the FunctionTemplateInfo used to create the
constructor function that says "this type participates in tracing", and only
record those

https://codereview.chromium.org/1844413002/diff/1/src/heap/mark-compact.cc#ne...
src/heap/mark-compact.cc:1925:
reinterpret_cast<void*>(js_object->GetInternalField(0)),
these can be potentially "undefined_value()", right?

https://codereview.chromium.org/1844413002/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1844413002/diff/1/src/objects.h#newcode2294
src/objects.h:2294: bool WasConstructedFromApiFunction();
can you also use this method in heap.cc:IsUnmodeifiedHeapObject?

[Marcel Hlopko, 2016-04-01 09:24:32]

https://codereview.chromium.org/1844413002/diff/1/src/heap/incremental-markin...
File src/heap/incremental-marking.cc (right):

https://codereview.chromium.org/1844413002/diff/1/src/heap/incremental-markin...
src/heap/incremental-marking.cc:738: if
(!heap_->mark_compact_collector()->UsingEmbedderHeapTracer()) {
Yes, but right now we aim for correctness of tracing, and I don't want object
groups to "help" me. But later, maybe when tracing will be on by default on
canary, we probably should leave both on (paying the price of performance
overhead and leaking memory), and turn groups off only after tracing proves
itself worthy :) Wdyt?

https://codereview.chromium.org/1844413002/diff/1/src/heap/mark-compact.cc
File src/heap/mark-compact.cc (right):

https://codereview.chromium.org/1844413002/diff/1/src/heap/mark-compact.cc#ne...
src/heap/mark-compact.cc:1923: js_object->WasConstructedFromApiFunction()) {
Noted.

https://codereview.chromium.org/1844413002/diff/1/src/heap/mark-compact.cc#ne...
src/heap/mark-compact.cc:1925:
reinterpret_cast<void*>(js_object->GetInternalField(0)),
Yes, I guarded against that in blink side, here it makes more sense. Done.

https://codereview.chromium.org/1844413002/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1844413002/diff/1/src/objects.h#newcode2294
src/objects.h:2294: bool WasConstructedFromApiFunction();
Indeed, done.

[Hannes Payer (out of office), 2016-04-01 11:13:37]

https://codereview.chromium.org/1844413002/diff/20001/src/heap/mark-compact.cc
File src/heap/mark-compact.cc (right):

https://codereview.chromium.org/1844413002/diff/20001/src/heap/mark-compact.c...
src/heap/mark-compact.cc:1919: if (UsingEmbedderHeapTracer() &&
object->IsJSObject()) {
As discussed offline, move this code to the proper object visitor which gets
called by IterateBody.

https://codereview.chromium.org/1844413002/diff/20001/src/heap/mark-compact.c...
src/heap/mark-compact.cc:1926: wrappers_to_trace().push_back(std::pair<void*,
void*>(
Could these be Object*?

https://codereview.chromium.org/1844413002/diff/20001/src/heap/mark-compact.c...
src/heap/mark-compact.cc:1988: v8::Isolate* api_isolate =
reinterpret_cast<v8::Isolate*>(isolate());
This method now looks really complicated. Can we decouple these two modes?

[Marcel Hlopko, 2016-04-01 12:47:20]

Incorporated Hannes' comments, the cl is a little bit bigger now. Would you mind
commenting on whether I found correct place to hook into in visiting, and your
opinion on the mark-compact.cc refactoring? Thanks!

https://codereview.chromium.org/1844413002/diff/20001/src/heap/mark-compact.cc
File src/heap/mark-compact.cc (right):

https://codereview.chromium.org/1844413002/diff/20001/src/heap/mark-compact.c...
src/heap/mark-compact.cc:1919: if (UsingEmbedderHeapTracer() &&
object->IsJSObject()) {
On 2016/04/01 at 11:13:36, Hannes Payer wrote:
> As discussed offline, move this code to the proper object visitor which gets
called by IterateBody.

Gave it a try, my tests still pass, so I hope I found a correct place, but ptal
to be sure.

https://codereview.chromium.org/1844413002/diff/20001/src/heap/mark-compact.c...
src/heap/mark-compact.cc:1926: wrappers_to_trace().push_back(std::pair<void*,
void*>(
On 2016/04/01 at 11:13:36, Hannes Payer wrote:
> Could these be Object*?

I'm afraid they cannot, from the v8 side they are just smis, just accidentally
pointing to the blink objects (but v8 doesn't know that).

https://codereview.chromium.org/1844413002/diff/20001/src/heap/mark-compact.c...
src/heap/mark-compact.cc:1988: v8::Isolate* api_isolate =
reinterpret_cast<v8::Isolate*>(isolate());
On 2016/04/01 at 11:13:36, Hannes Payer wrote:
> This method now looks really complicated. Can we decouple these two modes?

I was not happy with the split into 2 methods, so I splitted it into 3... Wdyt?

[Marcel Hlopko, 2016-04-04 09:32:12]

Realized objects reachable from weak harmony collection elements marked for
destruction can reach new wrappers, which need to be processed (au contraire to
object groups).

I see that I distort the GCTracer scopes a little, I should probably introduce a
different scope for embedder heap tracing, wdyt? Do you want it to be a part of
this (arguably already too big) cl?

[Marcel Hlopko, 2016-04-06 10:10:25]

Introduced new object type - JSApiObject. Ptal :)

[Marcel Hlopko, 2016-04-06 12:08:26]

hlopko@chromium.org changed reviewers:
+ toonaest@chromium.org

[Marcel Hlopko, 2016-04-06 12:09:03]

Ptal :)

[Marcel Hlopko, 2016-04-06 12:18:19]

hlopko@chromium.org changed reviewers:
+ verwaest@chromium.org
- toonaest@chromium.org

[Marcel Hlopko, 2016-04-06 13:15:20]

Patchset #8 (id:140001) has been deleted

[Marcel Hlopko, 2016-04-06 13:39:46]

So it looks like I fixed the tests, so you can take a look. Thanks!

[Toon Verwaest, 2016-04-06 14:07:48]

Adding the instance type lgtm

https://codereview.chromium.org/1844413002/diff/160001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1844413002/diff/160001/src/objects.cc#newcode...
src/objects.cc:15991: bool JSObject::WasConstructedFromApiFunction() {
can't we now just check == JS_API... || JS_SPECIAL_API...? GetConstructor below
is pretty slow.

[Marcel Hlopko, 2016-04-07 08:00:15]

Description was changed from

==========
Use EmbedderHeapTracer instead of object grouping when trace_embedder_heap flag
is set

BUG=chromium:468240
LOG=no
==========

to

==========
Use EmbedderHeapTracer instead of object grouping when trace_embedder_heap flag
is set.

When a flag is set, V8, during marking, will collect all reachable wrappers, and
then ask embedder to trace its heap. The embedder is expected to call
PersistentBase::RegisterExternalReference with all wrappers reachable from the
given ones. This fixed point iteration happens in
MarkCompact::ProcessEphemeralMarking.

For more efficient object visiting during marking, we need a special
JS_API_OBJECT_TYPE (in tandem with already existing JS_SPECIAL_API_OBJECT_TYPE)
and corresponding visitor (JSApiObjectVisitor).

BUG=chromium:468240
LOG=no
==========

[Marcel Hlopko, 2016-04-07 08:09:53]

https://codereview.chromium.org/1844413002/diff/160001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1844413002/diff/160001/src/objects.cc#newcode...
src/objects.cc:15991: bool JSObject::WasConstructedFromApiFunction() {
On 2016/04/06 at 14:07:47, Toon Verwaest wrote:
> can't we now just check == JS_API... || JS_SPECIAL_API...? GetConstructor
below is pretty slow.

Yes indeed. I left the slow path hidden behind FLAG_enable_slow_asserts (hope
this is correct usage of the flag).

[Hannes Payer (out of office), 2016-04-07 20:38:31]

Overall looking good. A few more comments.

https://codereview.chromium.org/1844413002/diff/220001/src/heap/heap.h
File src/heap/heap.h (right):

https://codereview.chromium.org/1844413002/diff/220001/src/heap/heap.h#newcod...
src/heap/heap.h:920: EmbedderHeapTracer* embedder_heap_tracer() { return
embedder_heap_tracer_; }
Since the tracer is fully used in mark-compact, it should actually live there.

https://codereview.chromium.org/1844413002/diff/220001/src/heap/heap.h#newcod...
src/heap/heap.h:922: bool UsingEmbedderHeapTracer() { return
embedder_heap_tracer_; }
Why don't we just check FLAG_trace_object_groups?

https://codereview.chromium.org/1844413002/diff/220001/src/heap/incremental-m...
File src/heap/incremental-marking.cc (right):

https://codereview.chromium.org/1844413002/diff/220001/src/heap/incremental-m...
src/heap/incremental-marking.cc:730: MarkObjectGroups();
Can you check FLAG_trace_object_groups here and DCHECK in MarkObjectGroups for
DCHECK(!heap_->UsingEmbedderHeapTracer())

https://codereview.chromium.org/1844413002/diff/220001/src/heap/mark-compact.cc
File src/heap/mark-compact.cc (right):

https://codereview.chromium.org/1844413002/diff/220001/src/heap/mark-compact....
src/heap/mark-compact.cc:2140: if (heap()->UsingEmbedderHeapTracer()) {
This piece of code and the code below need separate timer scopes.

[jochen (gone - plz use gerrit), 2016-04-08 02:19:56]

https://codereview.chromium.org/1844413002/diff/220001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/1844413002/diff/220001/include/v8.h#newcode5400
include/v8.h:5400: const std::vector<std::pair<Value*, Value*> >&
internal_fields) = 0;
why this change? I don't think we should expose Data* as that's internally
expected to be a Handle<Object> which this is not.

[Marcel Hlopko, 2016-04-08 08:08:10]

https://codereview.chromium.org/1844413002/diff/220001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/1844413002/diff/220001/include/v8.h#newcode5400
include/v8.h:5400: const std::vector<std::pair<Value*, Value*> >&
internal_fields) = 0;
On 2016/04/08 at 02:19:56, jochen - slow wrote:
> why this change? I don't think we should expose Data* as that's internally
expected to be a Handle<Object> which this is not.

The motivation was to be consistent with SetInternalField, which takes
Local<Value>. I had an impression that Value represents i::Object, not
Handle<i::Object>, now I see I was wrong :) Switching back to void*.

https://codereview.chromium.org/1844413002/diff/220001/src/heap/heap.h
File src/heap/heap.h (right):

https://codereview.chromium.org/1844413002/diff/220001/src/heap/heap.h#newcod...
src/heap/heap.h:920: EmbedderHeapTracer* embedder_heap_tracer() { return
embedder_heap_tracer_; }
Done.

https://codereview.chromium.org/1844413002/diff/220001/src/heap/heap.h#newcod...
src/heap/heap.h:922: bool UsingEmbedderHeapTracer() { return
embedder_heap_tracer_; }
On 2016/04/07 at 20:38:30, Hannes Payer wrote:
> Why don't we just check FLAG_trace_object_groups?

I removed the flag (or to be more accurrate, moved it to chromium). The embedder
heap tracing feature is now controlled only by the presence of the heap tracer.
If embedder sets it, v8 will use it. If it's nullptr, v8 will use object
grouping.

https://codereview.chromium.org/1844413002/diff/220001/src/heap/incremental-m...
File src/heap/incremental-marking.cc (right):

https://codereview.chromium.org/1844413002/diff/220001/src/heap/incremental-m...
src/heap/incremental-marking.cc:730: MarkObjectGroups();
Done.

https://codereview.chromium.org/1844413002/diff/220001/src/heap/mark-compact.cc
File src/heap/mark-compact.cc (right):

https://codereview.chromium.org/1844413002/diff/220001/src/heap/mark-compact....
src/heap/mark-compact.cc:2140: if (heap()->UsingEmbedderHeapTracer()) {
On 2016/04/07 at 20:38:31, Hannes Payer wrote:
> This piece of code and the code below need separate timer scopes.

Let's not make this method more complicated than it already is. We can do it
later if we'll need more precise numbers than what we have now.

[Marcel Hlopko, 2016-04-08 08:14:09]

Description was changed from

==========
Use EmbedderHeapTracer instead of object grouping when trace_embedder_heap flag
is set.

When a flag is set, V8, during marking, will collect all reachable wrappers, and
then ask embedder to trace its heap. The embedder is expected to call
PersistentBase::RegisterExternalReference with all wrappers reachable from the
given ones. This fixed point iteration happens in
MarkCompact::ProcessEphemeralMarking.

For more efficient object visiting during marking, we need a special
JS_API_OBJECT_TYPE (in tandem with already existing JS_SPECIAL_API_OBJECT_TYPE)
and corresponding visitor (JSApiObjectVisitor).

BUG=chromium:468240
LOG=no
==========

to

==========
Use EmbedderHeapTracer instead of object grouping when embedder sets the heap
tracer.

When the embedder sets the heap tracer, V8, during marking, will collect all
reachable wrappers, and then ask embedder to trace its heap. The embedder is
expected to call PersistentBase::RegisterExternalReference with all wrappers
reachable from the given ones. This fixed point iteration happens in
MarkCompact::ProcessEphemeralMarking.

For more efficient object visiting during marking, we need a special
JS_API_OBJECT_TYPE (in tandem with already existing JS_SPECIAL_API_OBJECT_TYPE)
and corresponding visitor (JSApiObjectVisitor).

BUG=chromium:468240
LOG=no
==========

[Hannes Payer (out of office), 2016-04-08 12:02:09]

one nit left, LGTM from my side

https://codereview.chromium.org/1844413002/diff/260001/src/heap/heap.h
File src/heap/heap.h (right):

https://codereview.chromium.org/1844413002/diff/260001/src/heap/heap.h#newcod...
src/heap/heap.h:915: // Embedder heap tracing.
====================================================
Since all components are just used by the mark compact collector, we can just
remove the API completely and just forward everything to
mark_compact_collector()

[jochen (gone - plz use gerrit), 2016-04-09 23:46:43]

lgtm

[Marcel Hlopko, 2016-04-11 10:29:22]

Final version, will be landed once v8 5.1 branch is stable and the queue is open
for new stuff again.

https://codereview.chromium.org/1844413002/diff/260001/src/heap/heap.h
File src/heap/heap.h (right):

https://codereview.chromium.org/1844413002/diff/260001/src/heap/heap.h#newcod...
src/heap/heap.h:915: // Embedder heap tracing.
====================================================
On 2016/04/08 at 12:02:09, Hannes Payer wrote:
> Since all components are just used by the mark compact collector, we can just
remove the API completely and just forward everything to
mark_compact_collector()

Done.

[Marcel Hlopko, 2016-04-12 10:08:36]

The CQ bit was checked by hlopko@chromium.org

[Marcel Hlopko, 2016-04-12 10:08:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
verwaest@chromium.org, hpayer@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1844413002/#ps280001
(title: "Move logic from heap to mark compact")

[commit-bot: I haz the power, 2016-04-12 10:08:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1844413002/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1844413002/280001

[commit-bot: I haz the power, 2016-04-12 10:31:43]

Committed patchset #14 (id:280001)

[commit-bot: I haz the power, 2016-04-12 10:33:35]

Description was changed from

==========
Use EmbedderHeapTracer instead of object grouping when embedder sets the heap
tracer.

When the embedder sets the heap tracer, V8, during marking, will collect all
reachable wrappers, and then ask embedder to trace its heap. The embedder is
expected to call PersistentBase::RegisterExternalReference with all wrappers
reachable from the given ones. This fixed point iteration happens in
MarkCompact::ProcessEphemeralMarking.

For more efficient object visiting during marking, we need a special
JS_API_OBJECT_TYPE (in tandem with already existing JS_SPECIAL_API_OBJECT_TYPE)
and corresponding visitor (JSApiObjectVisitor).

BUG=chromium:468240
LOG=no
==========

to

==========
Use EmbedderHeapTracer instead of object grouping when embedder sets the heap
tracer.

When the embedder sets the heap tracer, V8, during marking, will collect all
reachable wrappers, and then ask embedder to trace its heap. The embedder is
expected to call PersistentBase::RegisterExternalReference with all wrappers
reachable from the given ones. This fixed point iteration happens in
MarkCompact::ProcessEphemeralMarking.

For more efficient object visiting during marking, we need a special
JS_API_OBJECT_TYPE (in tandem with already existing JS_SPECIAL_API_OBJECT_TYPE)
and corresponding visitor (JSApiObjectVisitor).

BUG=chromium:468240
LOG=no

Committed: https://crrev.com/6d1f7282aff7cf332bbecf3f63e99b2e7716fa14
Cr-Commit-Position: refs/heads/master@{#35412}
==========

[commit-bot: I haz the power, 2016-04-12 10:33:36]

Patchset 14 (id:??) landed as
https://crrev.com/6d1f7282aff7cf332bbecf3f63e99b2e7716fa14
Cr-Commit-Position: refs/heads/master@{#35412}