Add flag to trace object groups

Add flag to trace object groups
BUG=chromium:468240
LOG=no

Committed: https://crrev.com/fd43661c501b63e05f02d870ec0ed7f6db171d62
Cr-Commit-Position: refs/heads/master@{#34634}

[Marcel Hlopko, 2016-03-08 20:16:43]

Description was changed from

==========
Add flag to trace object groups
BUG=chromium:468240
LOG=no
==========

to

==========
Add flag to trace object groups
BUG=chromium:468240
LOG=no
==========

[Marcel Hlopko, 2016-03-08 20:16:44]

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

[Marcel Hlopko, 2016-03-08 20:17:13]

ptal

[ulan, 2016-03-08 20:23:36]

Nice! Few nits. I'll do proper review tomorrow.

https://codereview.chromium.org/1773273002/diff/1/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/1773273002/diff/1/src/flag-definitions.h#newc...
src/flag-definitions.h:732: // object-groups-tracer.cc
Should be global-handles.cc

https://codereview.chromium.org/1773273002/diff/1/src/global-handles.cc
File src/global-handles.cc (right):

https://codereview.chromium.org/1773273002/diff/1/src/global-handles.cc#newco...
src/global-handles.cc:773: ObjectGroupsTracer(isolate_).CollectTraces();
Not needed anymore.

https://codereview.chromium.org/1773273002/diff/1/src/global-handles.h
File src/global-handles.h (right):

https://codereview.chromium.org/1773273002/diff/1/src/global-handles.h#newcod...
src/global-handles.h:361: friend class ObjectGroupsTracer;
Not needed.

[Marcel Hlopko, 2016-03-09 09:30:30]

ptal, v2 :)

https://codereview.chromium.org/1773273002/diff/1/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/1773273002/diff/1/src/flag-definitions.h#newc...
src/flag-definitions.h:732: // object-groups-tracer.cc
On 2016/03/08 20:23:32, ulan wrote:
> Should be global-handles.cc

Done.

https://codereview.chromium.org/1773273002/diff/1/src/global-handles.cc
File src/global-handles.cc (right):

https://codereview.chromium.org/1773273002/diff/1/src/global-handles.cc#newco...
src/global-handles.cc:773: ObjectGroupsTracer(isolate_).CollectTraces();
On 2016/03/08 20:23:32, ulan wrote:
> Not needed anymore.

Done.

https://codereview.chromium.org/1773273002/diff/1/src/global-handles.h
File src/global-handles.h (right):

https://codereview.chromium.org/1773273002/diff/1/src/global-handles.h#newcod...
src/global-handles.h:361: friend class ObjectGroupsTracer;
On 2016/03/08 20:23:33, ulan wrote:
> Not needed.

Done.

[Marcel Hlopko, 2016-03-09 12:32:56]

Simplified the code after Ulan's suggestions given offline

[ulan, 2016-03-09 14:37:49]

Few more nits.

https://codereview.chromium.org/1773273002/diff/60001/src/global-handles.cc
File src/global-handles.cc (right):

https://codereview.chromium.org/1773273002/diff/60001/src/global-handles.cc#n...
src/global-handles.cc:820: void CollectTraces();
Let's rename the method to Print() since that is what it does.

https://codereview.chromium.org/1773273002/diff/60001/src/global-handles.cc#n...
src/global-handles.cc:840: for (int i = 0; i < object_groups->length(); ++i) {
We can use:
for (auto group : *object_groups) {
..
}

https://codereview.chromium.org/1773273002/diff/60001/src/global-handles.cc#n...
src/global-handles.cc:853: void ObjectGroupsTracer::PrintObject(Object* o) {
Let's replace "o" with more descriptive "object".

https://codereview.chromium.org/1773273002/diff/60001/src/global-handles.cc#n...
src/global-handles.cc:854: JSObject* js_object = reinterpret_cast<JSObject*>(o);
if (object->IsJSObject()) {
  JSObject* js_object = JSObject::cast(object);
  ..
} else {
  PrintF("object of unexpected type: %p\n", object);
}

https://codereview.chromium.org/1773273002/diff/60001/src/global-handles.cc#n...
src/global-handles.cc:883: if (js_object->IsOddball()) {
Not needed.

https://codereview.chromium.org/1773273002/diff/60001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/1773273002/diff/60001/src/heap/heap.cc#newcod...
src/heap/heap.cc:831: if (FLAG_trace_object_groups) {
Let's move it in CallGCPrologueCallbacks and we need to check for GC type so
that we don't print groups for Scavenge GCs.

https://codereview.chromium.org/1773273002/diff/60001/src/heap/heap.cc#newcod...
src/heap/heap.cc:1291: if (FLAG_trace_object_groups) {
See comment above.

https://codereview.chromium.org/1773273002/diff/60001/src/heap/incremental-ma...
File src/heap/incremental-marking.cc (right):

https://codereview.chromium.org/1773273002/diff/60001/src/heap/incremental-ma...
src/heap/incremental-marking.cc:13: #include "src/heap/objects-visiting-inl.h"
Let's now change this file. Probably git cl format is confused here.

[Marcel Hlopko, 2016-03-09 17:20:23]

https://codereview.chromium.org/1773273002/diff/60001/src/global-handles.cc
File src/global-handles.cc (right):

https://codereview.chromium.org/1773273002/diff/60001/src/global-handles.cc#n...
src/global-handles.cc:820: void CollectTraces();
On 2016/03/09 14:37:49, ulan wrote:
> Let's rename the method to Print() since that is what it does.

Done.

https://codereview.chromium.org/1773273002/diff/60001/src/global-handles.cc#n...
src/global-handles.cc:840: for (int i = 0; i < object_groups->length(); ++i) {
On 2016/03/09 14:37:49, ulan wrote:
> We can use:
> for (auto group : *object_groups) {
> ..
> }

Done.

https://codereview.chromium.org/1773273002/diff/60001/src/global-handles.cc#n...
src/global-handles.cc:853: void ObjectGroupsTracer::PrintObject(Object* o) {
On 2016/03/09 14:37:49, ulan wrote:
> Let's replace "o" with more descriptive "object".

Done.

https://codereview.chromium.org/1773273002/diff/60001/src/global-handles.cc#n...
src/global-handles.cc:854: JSObject* js_object = reinterpret_cast<JSObject*>(o);
On 2016/03/09 14:37:49, ulan wrote:
> if (object->IsJSObject()) {
>   JSObject* js_object = JSObject::cast(object);
>   ..
> } else {
>   PrintF("object of unexpected type: %p\n", object);
> }

Done.

https://codereview.chromium.org/1773273002/diff/60001/src/global-handles.cc#n...
src/global-handles.cc:883: if (js_object->IsOddball()) {
On 2016/03/09 14:37:49, ulan wrote:
> Not needed.

Done.

https://codereview.chromium.org/1773273002/diff/60001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/1773273002/diff/60001/src/heap/heap.cc#newcod...
src/heap/heap.cc:831: if (FLAG_trace_object_groups) {
On 2016/03/09 14:37:49, ulan wrote:
> Let's move it in CallGCPrologueCallbacks and we need to check for GC type so
> that we don't print groups for Scavenge GCs.

Done.

https://codereview.chromium.org/1773273002/diff/60001/src/heap/heap.cc#newcod...
src/heap/heap.cc:1291: if (FLAG_trace_object_groups) {
On 2016/03/09 14:37:49, ulan wrote:
> See comment above.

Done.

https://codereview.chromium.org/1773273002/diff/60001/src/heap/incremental-ma...
File src/heap/incremental-marking.cc (right):

https://codereview.chromium.org/1773273002/diff/60001/src/heap/incremental-ma...
src/heap/incremental-marking.cc:13: #include "src/heap/objects-visiting-inl.h"
On 2016/03/09 14:37:49, ulan wrote:
> Let's now change this file. Probably git cl format is confused here.

Done.

[ulan, 2016-03-09 17:23:20]

lgtm

[Marcel Hlopko, 2016-03-09 17:23:44]

The CQ bit was checked by hlopko@chromium.org

[commit-bot: I haz the power, 2016-03-09 17:23:47]

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

[commit-bot: I haz the power, 2016-03-09 17:54:14]

Description was changed from

==========
Add flag to trace object groups
BUG=chromium:468240
LOG=no
==========

to

==========
Add flag to trace object groups
BUG=chromium:468240
LOG=no
==========

[commit-bot: I haz the power, 2016-03-09 17:54:15]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-03-09 17:56:23]

Description was changed from

==========
Add flag to trace object groups
BUG=chromium:468240
LOG=no
==========

to

==========
Add flag to trace object groups
BUG=chromium:468240
LOG=no

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

[commit-bot: I haz the power, 2016-03-09 17:56:24]

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

[Marcel Hlopko, 2016-03-09 18:57:48]

On 2016/03/09 17:56:24, commit-bot: I haz the power wrote:
> Patchset 6 (id:??) landed as
> https://crrev.com/fd43661c501b63e05f02d870ec0ed7f6db171d62
> Cr-Commit-Position: refs/heads/master@{#34634}

yay :)