[wrapper-tracing] Snapshot: Avoid adding non-Node wrappers to groups

[wrapper-tracing] Snapshot: Avoid adding non-Node wrappers to groups

Tests currently rely on the fact that only Nodes are part of retainer
groups.

Code paths will only be exercised once 
  https://codereview.chromium.org/2627033002/
on the V8 side lands.

BUG=chromium:679724
NOTRY=true

Review-Url: https://codereview.chromium.org/2651923002
Cr-Commit-Position: refs/heads/master@{#445694}
Committed: https://chromium.googlesource.com/chromium/src/+/166ffeb0e2f6c7175f1e0988d145d57c30406e0f

[Michael Lippautz, 2017-01-24 10:09:46]

Description was changed from

==========
[wrapper-tracing] Avoid adding non-Node wrappers to groups

Tests currently rely on the fact that only Nodes are part of retainer
groups.

BUG=
==========

to

==========
[wrapper-tracing] Avoid adding non-Node wrappers to groups

Tests currently rely on the fact that only Nodes are part of retainer
groups.

BUG=chromium:679724
==========

[Michael Lippautz, 2017-01-24 10:11:01]

mlippautz@chromium.org changed reviewers:
+ jochen@chromium.org

[jochen (gone - plz use gerrit), 2017-01-24 10:12:05]

lgtm

[Michael Lippautz, 2017-01-24 10:16:27]

Description was changed from

==========
[wrapper-tracing] Avoid adding non-Node wrappers to groups

Tests currently rely on the fact that only Nodes are part of retainer
groups.

BUG=chromium:679724
==========

to

==========
[wrapper-tracing] Avoid adding non-Node wrappers to groups

Tests currently rely on the fact that only Nodes are part of retainer
groups.

Code paths will only be exercised once 
  https://codereview.chromium.org/2627033002/
lands.

BUG=chromium:679724
NOTRY=true
==========

[Michael Lippautz, 2017-01-24 10:16:38]

Description was changed from

==========
[wrapper-tracing] Avoid adding non-Node wrappers to groups

Tests currently rely on the fact that only Nodes are part of retainer
groups.

Code paths will only be exercised once 
  https://codereview.chromium.org/2627033002/
lands.

BUG=chromium:679724
NOTRY=true
==========

to

==========
[wrapper-tracing] Avoid adding non-Node wrappers to groups

Tests currently rely on the fact that only Nodes are part of retainer
groups.

Code paths will only be exercised once 
  https://codereview.chromium.org/2627033002/
on the V8 side lands.

BUG=chromium:679724
NOTRY=true
==========

[Michael Lippautz, 2017-01-24 10:22:45]

Description was changed from

==========
[wrapper-tracing] Avoid adding non-Node wrappers to groups

Tests currently rely on the fact that only Nodes are part of retainer
groups.

Code paths will only be exercised once 
  https://codereview.chromium.org/2627033002/
on the V8 side lands.

BUG=chromium:679724
NOTRY=true
==========

to

==========
[wrapper-tracing] Snapshot: Avoid adding non-Node wrappers to groups

Tests currently rely on the fact that only Nodes are part of retainer
groups.

Code paths will only be exercised once 
  https://codereview.chromium.org/2627033002/
on the V8 side lands.

BUG=chromium:679724
NOTRY=true
==========

[Michael Lippautz, 2017-01-24 10:37:06]

The CQ bit was checked by mlippautz@chromium.org

[Michael Lippautz, 2017-01-24 10:37:07]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/2651923002/#ps20001
(title: "Fix typo")

[commit-bot: I haz the power, 2017-01-24 10:37:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-24 10:41:18]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1485254226954650,
"parent_rev": "eb89fbc6b3248eb435b28e0e2a04b2475ccea184", "commit_rev":
"166ffeb0e2f6c7175f1e0988d145d57c30406e0f"}

[commit-bot: I haz the power, 2017-01-24 10:41:52]

Description was changed from

==========
[wrapper-tracing] Snapshot: Avoid adding non-Node wrappers to groups

Tests currently rely on the fact that only Nodes are part of retainer
groups.

Code paths will only be exercised once 
  https://codereview.chromium.org/2627033002/
on the V8 side lands.

BUG=chromium:679724
NOTRY=true
==========

to

==========
[wrapper-tracing] Snapshot: Avoid adding non-Node wrappers to groups

Tests currently rely on the fact that only Nodes are part of retainer
groups.

Code paths will only be exercised once 
  https://codereview.chromium.org/2627033002/
on the V8 side lands.

BUG=chromium:679724
NOTRY=true

Review-Url: https://codereview.chromium.org/2651923002
Cr-Commit-Position: refs/heads/master@{#445694}
Committed:
https://chromium.googlesource.com/chromium/src/+/166ffeb0e2f6c7175f1e0988d145...
==========

[commit-bot: I haz the power, 2017-01-24 10:41:53]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/166ffeb0e2f6c7175f1e0988d145...

[haraken, 2017-01-24 21:04:58]

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

[haraken, 2017-01-24 21:04:58]

LGTM.

> Tests currently rely on the fact that only Nodes are part of retainer
groups.

I'd say that the test should be updated though. Conceptually it should be better
if we could list up all reachable wrappers, not limited to Nodes.

https://codereview.chromium.org/2651923002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp (right):

https://codereview.chromium.org/2651923002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:209:
findV8WrappersDirectlyReachableFrom(node);

Now, isn't it useless to call findV8WrappersDirectlyReachableFrom?

[Michael Lippautz, 2017-01-24 21:45:13]

On 2017/01/24 21:04:58, haraken wrote:
> LGTM.
> 
> > Tests currently rely on the fact that only Nodes are part of retainer
> groups.
> 
> I'd say that the test should be updated though. Conceptually it should be
better
> if we could list up all reachable wrappers, not limited to Nodes.
> 

Totally agree!

I pushed the related changes now through for several reasons
- The port is complete, meaning that we are unblocked from removing object
grouping code from blink. I will prepare further CLs in the following days.
- With the port being complete, we can now change the behavior on the blink side
including the tests, without modifying any V8 code (unless I am somehow
mistaken). I think this makes introducing changes a lot easier as you can avoid
the API dance.
- We might have an API freeze rather soon to allow launching some other V8
features. I wanted to be sure that the profiler changes are all landed.

Additionally, I might no be the right person on deciding what semantics we
should have in the snapshot profiler :)

>
https://codereview.chromium.org/2651923002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp (right):
> 
>
https://codereview.chromium.org/2651923002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:209:
> findV8WrappersDirectlyReachableFrom(node);
> 
> Now, isn't it useless to call findV8WrappersDirectlyReachableFrom?

It still collects edge information which is used by the profiler. E.g.
EventListeners that are attached to a DOM node.

[haraken, 2017-01-24 21:47:51]

On 2017/01/24 21:45:13, Michael Lippautz wrote:
> On 2017/01/24 21:04:58, haraken wrote:
> > LGTM.
> > 
> > > Tests currently rely on the fact that only Nodes are part of retainer
> > groups.
> > 
> > I'd say that the test should be updated though. Conceptually it should be
> better
> > if we could list up all reachable wrappers, not limited to Nodes.
> > 
> 
> Totally agree!
> 
> I pushed the related changes now through for several reasons
> - The port is complete, meaning that we are unblocked from removing object
> grouping code from blink. I will prepare further CLs in the following days.
> - With the port being complete, we can now change the behavior on the blink
side
> including the tests, without modifying any V8 code (unless I am somehow
> mistaken). I think this makes introducing changes a lot easier as you can
avoid
> the API dance.
> - We might have an API freeze rather soon to allow launching some other V8
> features. I wanted to be sure that the profiler changes are all landed.
> 
> Additionally, I might no be the right person on deciding what semantics we
> should have in the snapshot profiler :)
> 
> >
>
https://codereview.chromium.org/2651923002/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2651923002/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:209:
> > findV8WrappersDirectlyReachableFrom(node);
> > 
> > Now, isn't it useless to call findV8WrappersDirectlyReachableFrom?
> 
> It still collects edge information which is used by the profiler. E.g.
> EventListeners that are attached to a DOM node.

Thanks, makes sense.