Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2625093002:
[wrapper-tracing] Add heap snapshot generator infrastructure (Closed)
|
Created:
3 years, 11 months ago by Michael Lippautz Modified:
3 years, 11 months ago CC:
Marcel Hlopko, blink-reviews, blink-reviews-bindings_chromium.org, chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[wrapper-tracing] Add heap snapshot generator infrastructure
Register a callback with V8's heap profiler that can be used to query all
RetainedObjectInfo_s for a given Isolate.
Basically, we create RetainedObjectInfo_s for DOM trees (attached and detached)
and pending activities and assign all found wrappers to these info objects.
BUG=chromium:679724
Review-Url: https://codereview.chromium.org/2625093002
Cr-Commit-Position: refs/heads/master@{#445372}
Committed: https://chromium.googlesource.com/chromium/src/+/a34d185b3e31187fb8aa16b700e5837e9636cba9
Patch Set 1 #Patch Set 2 : Move the scope to V8PerIsolateData #
Total comments: 2
Patch Set 3 : Add processing of DOM trees #Patch Set 4 : Fix leftover #
Total comments: 18
Patch Set 5 : Rework and address comments #
Total comments: 35
Patch Set 6 : Addressed comments #
Total comments: 4
Patch Set 7 : Addressed comments #Patch Set 8 : Also trace through empty ScriptWrappable #Patch Set 9 : Rebase and fix CHECK #Messages
Total messages: 55 (26 generated)
Description was changed from ========== [wrapper-tracing] Add heap snapshot generator infrastructure BUG= ========== to ========== [wrapper-tracing] Add heap snapshot generator infrastructure BUG= ==========
mlippautz@chromium.org changed reviewers: + alph@chromium.org, haraken@chromium.org, jochen@chromium.org
Description was changed from ========== [wrapper-tracing] Add heap snapshot generator infrastructure BUG= ========== to ========== [wrapper-tracing] Add heap snapshot generator infrastructure BUG= ==========
mlippautz@chromium.org changed reviewers: + alph@chromium.org, haraken@chromium.org, jochen@chromium.org
ptal V8: https://codereview.chromium.org/2627033002/ It's not yet complete, i.e., it only returns the infos for the pending activities. I wanted to get early feedback though as filling in the rest should be trivial if the approach is fine.
Description was changed from ========== [wrapper-tracing] Add heap snapshot generator infrastructure BUG= ========== to ========== [wrapper-tracing] Add heap snapshot generator infrastructure Register a callback with V8's heap profiler that can be used to query all RetainedObjectInfo_s for a given Isolate. Basically, we create RetainedObjectInfo_s for DOM trees (attached and detached) and pending activities and assign all found wrappers to these info objects. BUG=679724 ==========
Description was changed from ========== [wrapper-tracing] Add heap snapshot generator infrastructure Register a callback with V8's heap profiler that can be used to query all RetainedObjectInfo_s for a given Isolate. Basically, we create RetainedObjectInfo_s for DOM trees (attached and detached) and pending activities and assign all found wrappers to these info objects. BUG=679724 ========== to ========== [wrapper-tracing] Add heap snapshot generator infrastructure Register a callback with V8's heap profiler that can be used to query all RetainedObjectInfo_s for a given Isolate. Basically, we create RetainedObjectInfo_s for DOM trees (attached and detached) and pending activities and assign all found wrappers to these info objects. BUG=679724 ==========
Description was changed from ========== [wrapper-tracing] Add heap snapshot generator infrastructure Register a callback with V8's heap profiler that can be used to query all RetainedObjectInfo_s for a given Isolate. Basically, we create RetainedObjectInfo_s for DOM trees (attached and detached) and pending activities and assign all found wrappers to these info objects. BUG=679724 ========== to ========== [wrapper-tracing] Add heap snapshot generator infrastructure Register a callback with V8's heap profiler that can be used to query all RetainedObjectInfo_s for a given Isolate. Basically, we create RetainedObjectInfo_s for DOM trees (attached and detached) and pending activities and assign all found wrappers to these info objects. BUG=679724 ==========
(Side note: I am actually quite happy to see how flexible the tracing infra is by now.)
The approach looks good, although TemporaryScriptWrappableVisitorScope looks weird. Can we implement a stack of ScriptWrappableVisitor in V8PerIsolateData? e.g., you can implement: - V8PerIsolateData::currentWrappableVisitor - V8PerIsolateData::pushWrappableVisitor - V8PerIsolateData::popWrappableVisitor The push/pop can be encapsulated in V8PerIsolateData::ScriptWrappableVisitorScope or something if necessary.
On 2017/01/11 12:22:24, haraken wrote:
> The approach looks good, although TemporaryScriptWrappableVisitorScope looks
> weird.
>
> Can we implement a stack of ScriptWrappableVisitor in V8PerIsolateData? e.g.,
> you can implement:
>
> - V8PerIsolateData::currentWrappableVisitor
> - V8PerIsolateData::pushWrappableVisitor
> - V8PerIsolateData::popWrappableVisitor
>
> The push/pop can be encapsulated in
> V8PerIsolateData::ScriptWrappableVisitorScope or something if necessary.
TemporaryScriptWrappableVisitorScope should only be stack allocated (I am
missing the STACK_ALLOCATED() macro). That already provides us with a stack,
e.g.,
{
TemporaryScriptWrappableVisitorScope scope1(...);
{
TemporaryScriptWrappableVisitorScope scope2(...);
// Using tracer passed to scope2.
}
// Using tracer passed to scope1.
}
On 2017/01/11 12:29:46, Michael Lippautz (OOO) wrote:
> On 2017/01/11 12:22:24, haraken wrote:
> > The approach looks good, although TemporaryScriptWrappableVisitorScope looks
> > weird.
> >
> > Can we implement a stack of ScriptWrappableVisitor in V8PerIsolateData?
e.g.,
> > you can implement:
> >
> > - V8PerIsolateData::currentWrappableVisitor
> > - V8PerIsolateData::pushWrappableVisitor
> > - V8PerIsolateData::popWrappableVisitor
> >
> > The push/pop can be encapsulated in
> > V8PerIsolateData::ScriptWrappableVisitorScope or something if necessary.
>
> TemporaryScriptWrappableVisitorScope should only be stack allocated (I am
> missing the STACK_ALLOCATED() macro). That already provides us with a stack,
> e.g.,
>
> {
> TemporaryScriptWrappableVisitorScope scope1(...);
> {
> TemporaryScriptWrappableVisitorScope scope2(...);
> // Using tracer passed to scope2.
> }
> // Using tracer passed to scope1.
> }
Jochen pointed out that moving the *Scope class to V8PerIsolateData would allow
us to get rid of the weird resetting of handles. I will do that and make it
clear that this is to be used on the stack.
On 2017/01/11 12:29:46, Michael Lippautz (OOO) wrote:
> On 2017/01/11 12:22:24, haraken wrote:
> > The approach looks good, although TemporaryScriptWrappableVisitorScope looks
> > weird.
> >
> > Can we implement a stack of ScriptWrappableVisitor in V8PerIsolateData?
e.g.,
> > you can implement:
> >
> > - V8PerIsolateData::currentWrappableVisitor
> > - V8PerIsolateData::pushWrappableVisitor
> > - V8PerIsolateData::popWrappableVisitor
> >
> > The push/pop can be encapsulated in
> > V8PerIsolateData::ScriptWrappableVisitorScope or something if necessary.
>
> TemporaryScriptWrappableVisitorScope should only be stack allocated (I am
> missing the STACK_ALLOCATED() macro). That already provides us with a stack,
> e.g.,
>
> {
> TemporaryScriptWrappableVisitorScope scope1(...);
> {
> TemporaryScriptWrappableVisitorScope scope2(...);
> // Using tracer passed to scope2.
> }
> // Using tracer passed to scope1.
> }
Yes, from the perspective of what should happen, the CL looks good, but the API
looks a bit nasty to me; e.g.,
setScriptWrappableVisitorWithoutDestroyingExisting.
Can we tidy it up by explicitly forming a stack of ScriptWrappableVisitor on
V8PerIsolateData and providing APIs for push/pop/current?
hlopko@chromium.org changed reviewers: + hlopko@chromium.org
Did you consider bringing back the reporter?
On 2017/01/11 12:41:38, Marcel Hlopko wrote: > Did you consider bringing back the reporter? Yes, I am on the edge. We still cannot move all the code to V8 then though as we would still need to create the RetainedObjectInfos, e.g., SuspendableObjectsInfo which is only known to blink. As we don't have another use case I think I would rather leave the tracing API as is for now.
Updated the TemporaryScriptWrappableVisitorScope.
lgtm
On 2017/01/12 08:31:46, jochen wrote: > lgtm I think you need to implement V8GCController::getRetainerInfos, right?
lgtm https://codereview.chromium.org/2625093002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp (right): https://codereview.chromium.org/2625093002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:198: mutable WTF::HashSet<const v8::PersistentBase<v8::Value>*> m_foundV8Wrappers; I wonder why not std::unordered_set ?
haraken: PTAL; All implemented now. https://codereview.chromium.org/2625093002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp (right): https://codereview.chromium.org/2625093002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:198: mutable WTF::HashSet<const v8::PersistentBase<v8::Value>*> m_foundV8Wrappers; On 2017/01/12 08:59:00, alph wrote: > I wonder why not std::unordered_set ? Done. Actually I changed the API type to unodred_set so that we can move rather than copy.
LGTM with comments. https://codereview.chromium.org/2625093002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp (right): https://codereview.chromium.org/2625093002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:54: #include <unordered_set> Remove this? You're using WTF::HashSet. https://codereview.chromium.org/2625093002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:202: class HeapSnapshotHandlesVisitor : public v8::PersistentHandleVisitor { In short term, this is fine, but I'm not sure if it's a good idea to identify a list of root nodes by HeapSnapshotHandlesVisitor. In theory, it's possible that there is a Node X such that X is reachable from the root set of a V8 GC but X doesn't have a wrapper. Then X won't be detected by v8::PersistentHandleVisitor. So, to get a full list of the root Nodes, we'll need to use the wrapper tracing instead of v8::PersistentHandleVisitor. https://codereview.chromium.org/2625093002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:210: classId != WrapperTypeInfo::ObjectClassId) Remove '&& classId != WrapperTypeInfo::ObjectClassId'. Here we're interested in only Nodes. https://codereview.chromium.org/2625093002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:213: if (value->IsIndependent()) DCHECK(!value->IsIndependent()). Nodes should be dependent objects. https://codereview.chromium.org/2625093002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:216: if (classId == WrapperTypeInfo::NodeClassId) { Remove the if statement. https://codereview.chromium.org/2625093002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:227: return std::move(m_nodesRequiringTracing); It might be clearer to use std::unique_ptr<WTF::HashSet<UntracedMember<Node>>>. Then you can return m_nodesRequiringTracing.get(). https://codereview.chromium.org/2625093002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:254: isolate->VisitHandlesWithClassIds(&visitor); Why can't we use |&tracer|? https://codereview.chromium.org/2625093002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.cpp (right): https://codereview.chromium.org/2625093002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.cpp:405: V8PerIsolateData::from(m_isolate)->m_scriptWrappableVisitor.release(); Do we need .release? Won't .reset() be enough? https://codereview.chromium.org/2625093002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.h (right): https://codereview.chromium.org/2625093002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.h:190: void swapInNewVisitor(ScriptWrappableVisitor*); Nit: swapInNewVisitor => setVisitor ? (Swap sounds like it swaps the two visitors.)
On 2017/01/12 16:25:18, haraken wrote: > LGTM with comments. > > https://codereview.chromium.org/2625093002/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp (right): > > https://codereview.chromium.org/2625093002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:54: #include > <unordered_set> > > Remove this? You're using WTF::HashSet. > > https://codereview.chromium.org/2625093002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:202: class > HeapSnapshotHandlesVisitor : public v8::PersistentHandleVisitor { > > In short term, this is fine, but I'm not sure if it's a good idea to identify a > list of root nodes by HeapSnapshotHandlesVisitor. > > In theory, it's possible that there is a Node X such that X is reachable from > the root set of a V8 GC but X doesn't have a wrapper. Then X won't be detected > by v8::PersistentHandleVisitor. So, to get a full list of the root Nodes, we'll > need to use the wrapper tracing instead of v8::PersistentHandleVisitor. > > https://codereview.chromium.org/2625093002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:210: classId != > WrapperTypeInfo::ObjectClassId) > > Remove '&& classId != WrapperTypeInfo::ObjectClassId'. Here we're interested in > only Nodes. > > https://codereview.chromium.org/2625093002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:213: if > (value->IsIndependent()) > > DCHECK(!value->IsIndependent()). Nodes should be dependent objects. > > https://codereview.chromium.org/2625093002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:216: if (classId > == WrapperTypeInfo::NodeClassId) { > > Remove the if statement. > > https://codereview.chromium.org/2625093002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:227: return > std::move(m_nodesRequiringTracing); > > It might be clearer to use std::unique_ptr<WTF::HashSet<UntracedMember<Node>>>. > Then you can return m_nodesRequiringTracing.get(). > > https://codereview.chromium.org/2625093002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:254: > isolate->VisitHandlesWithClassIds(&visitor); > > Why can't we use |&tracer|? > > https://codereview.chromium.org/2625093002/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.cpp (right): > > https://codereview.chromium.org/2625093002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.cpp:405: > V8PerIsolateData::from(m_isolate)->m_scriptWrappableVisitor.release(); > > Do we need .release? Won't .reset() be enough? > > https://codereview.chromium.org/2625093002/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.h (right): > > https://codereview.chromium.org/2625093002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.h:190: void > swapInNewVisitor(ScriptWrappableVisitor*); > > Nit: swapInNewVisitor => setVisitor ? > > (Swap sounds like it swaps the two visitors.) Thanks for the detailed review Kentaro! Unfortunately, I just (literally, the last hour) figured out that this approach just handles object groups and not implicit ref groups. I will update accordingly and incorporate your comments. In general, we still need the HeapSnapshotHandlesVisitor because it provides us with a notion of "DOM trees" which is apparently useful for developers. Wrapper tracing does't know about those trees, so we need to build it on the blink side. I will think about alternatives though.
On 2017/01/12 16:40:29, Michael Lippautz wrote: > On 2017/01/12 16:25:18, haraken wrote: > > LGTM with comments. > > > > > https://codereview.chromium.org/2625093002/diff/60001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp (right): > > > > > https://codereview.chromium.org/2625093002/diff/60001/third_party/WebKit/Sour... > > third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:54: #include > > <unordered_set> > > > > Remove this? You're using WTF::HashSet. > > > > > https://codereview.chromium.org/2625093002/diff/60001/third_party/WebKit/Sour... > > third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:202: class > > HeapSnapshotHandlesVisitor : public v8::PersistentHandleVisitor { > > > > In short term, this is fine, but I'm not sure if it's a good idea to identify > a > > list of root nodes by HeapSnapshotHandlesVisitor. > > > > In theory, it's possible that there is a Node X such that X is reachable from > > the root set of a V8 GC but X doesn't have a wrapper. Then X won't be detected > > by v8::PersistentHandleVisitor. So, to get a full list of the root Nodes, > we'll > > need to use the wrapper tracing instead of v8::PersistentHandleVisitor. > > > > > https://codereview.chromium.org/2625093002/diff/60001/third_party/WebKit/Sour... > > third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:210: classId != > > WrapperTypeInfo::ObjectClassId) > > > > Remove '&& classId != WrapperTypeInfo::ObjectClassId'. Here we're interested > in > > only Nodes. > > > > > https://codereview.chromium.org/2625093002/diff/60001/third_party/WebKit/Sour... > > third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:213: if > > (value->IsIndependent()) > > > > DCHECK(!value->IsIndependent()). Nodes should be dependent objects. > > > > > https://codereview.chromium.org/2625093002/diff/60001/third_party/WebKit/Sour... > > third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:216: if (classId > > == WrapperTypeInfo::NodeClassId) { > > > > Remove the if statement. > > > > > https://codereview.chromium.org/2625093002/diff/60001/third_party/WebKit/Sour... > > third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:227: return > > std::move(m_nodesRequiringTracing); > > > > It might be clearer to use > std::unique_ptr<WTF::HashSet<UntracedMember<Node>>>. > > Then you can return m_nodesRequiringTracing.get(). > > > > > https://codereview.chromium.org/2625093002/diff/60001/third_party/WebKit/Sour... > > third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:254: > > isolate->VisitHandlesWithClassIds(&visitor); > > > > Why can't we use |&tracer|? > > > > > https://codereview.chromium.org/2625093002/diff/60001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.cpp (right): > > > > > https://codereview.chromium.org/2625093002/diff/60001/third_party/WebKit/Sour... > > third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.cpp:405: > > V8PerIsolateData::from(m_isolate)->m_scriptWrappableVisitor.release(); > > > > Do we need .release? Won't .reset() be enough? > > > > > https://codereview.chromium.org/2625093002/diff/60001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.h (right): > > > > > https://codereview.chromium.org/2625093002/diff/60001/third_party/WebKit/Sour... > > third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.h:190: void > > swapInNewVisitor(ScriptWrappableVisitor*); > > > > Nit: swapInNewVisitor => setVisitor ? > > > > (Swap sounds like it swaps the two visitors.) > > Thanks for the detailed review Kentaro! > > Unfortunately, I just (literally, the last hour) figured out that this approach > just handles object groups and not implicit ref groups. I will update > accordingly and incorporate your comments. > > In general, we still need the HeapSnapshotHandlesVisitor because it provides us > with a notion of "DOM trees" which is apparently useful for developers. Wrapper > tracing does't know about those trees, so we need to build it on the blink side. > I will think about alternatives though. I agree that we need to build "DOM trees", but I think it's more correct to build the DOM trees using the wrapper tracing than using the object grouping. Would it be hard to collect a list of reachable Nodes using the wrapper tracing and build the DOM trees by calling opaqueRootForGC(node)?
Patchset #5 (id:70001) has been deleted
Please take another look. The implementation changed quite substantially. I will update the V8 side asap. It passes the tricky tests I know of: - Implicit references: inspector-protocol/heap-profiler/heap-snapshot-with-event-listener.html - Pending activities: inspector-protocol/heap-profiler/heap-snapshot-with-active-dom-object.html - Detached DOM trees: inspector-protocol/heap-profiler/heap-snapshot-with-detached-dom-tree.html This is a full heap graph now!! Compared to object grouping we already add all the edges we know of. The visualization lacks a bit, as we still visualize the groups for the DOM trees, but all the edges should be there. About where this visitor should live: I agree that on an abstract level it would make sense moving it to v8 but having spent 2 days now dealing with all sorts of wrinkles of our API, tracing, and what we would like to group, I think it is in the proper place. What is missing to move this to v8 is the ability in EmbedderHeapTracer to intercept tracing a single node (see dispatchTraceWrappers override). This is a bit tricky because of our transient types that have no wrapper associated. In short, I don't see a way to add edges from the V8 side. Having said that, I am open to revisiting that decision :) https://codereview.chromium.org/2625093002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp (right): https://codereview.chromium.org/2625093002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:54: #include <unordered_set> On 2017/01/12 16:25:17, haraken wrote: > > Remove this? You're using WTF::HashSet. I rewrote now everything using std:: types which can be easily passed across the API boundary. If you feel that we should use WTF:: during processing and only convert to std:: in the end just left me know. https://codereview.chromium.org/2625093002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:202: class HeapSnapshotHandlesVisitor : public v8::PersistentHandleVisitor { On 2017/01/12 16:25:17, haraken wrote: > > In short term, this is fine, but I'm not sure if it's a good idea to identify a > list of root nodes by HeapSnapshotHandlesVisitor. > > In theory, it's possible that there is a Node X such that X is reachable from > the root set of a V8 GC but X doesn't have a wrapper. Then X won't be detected > by v8::PersistentHandleVisitor. So, to get a full list of the root Nodes, we'll > need to use the wrapper tracing instead of v8::PersistentHandleVisitor. See general comment. https://codereview.chromium.org/2625093002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:210: classId != WrapperTypeInfo::ObjectClassId) On 2017/01/12 16:25:17, haraken wrote: > > Remove '&& classId != WrapperTypeInfo::ObjectClassId'. Here we're interested in > only Nodes. > Done. https://codereview.chromium.org/2625093002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:213: if (value->IsIndependent()) On 2017/01/12 16:25:17, haraken wrote: > > DCHECK(!value->IsIndependent()). Nodes should be dependent objects. Done. https://codereview.chromium.org/2625093002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:216: if (classId == WrapperTypeInfo::NodeClassId) { On 2017/01/12 16:25:17, haraken wrote: > > Remove the if statement. Done. https://codereview.chromium.org/2625093002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:227: return std::move(m_nodesRequiringTracing); On 2017/01/12 16:25:17, haraken wrote: > > It might be clearer to use std::unique_ptr<WTF::HashSet<UntracedMember<Node>>>. > Then you can return m_nodesRequiringTracing.get(). Doesn't apply anymore. https://codereview.chromium.org/2625093002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:254: isolate->VisitHandlesWithClassIds(&visitor); On 2017/01/12 16:25:17, haraken wrote: > > Why can't we use |&tracer|? Rewrote that tracer so that it implements both required interfaces (ScriptWrappableVisitor and v8::PersistentHandleVisitor). https://codereview.chromium.org/2625093002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.cpp (right): https://codereview.chromium.org/2625093002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.cpp:405: V8PerIsolateData::from(m_isolate)->m_scriptWrappableVisitor.release(); On 2017/01/12 16:25:17, haraken wrote: > > Do we need .release? Won't .reset() be enough? Yeah we need release, as the visitors used through TemporaryScriptWrappableVisitorScope can live on the stack. reset() also frees the visitor assigned to the unique ptr. https://codereview.chromium.org/2625093002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.h (right): https://codereview.chromium.org/2625093002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.h:190: void swapInNewVisitor(ScriptWrappableVisitor*); On 2017/01/12 16:25:18, haraken wrote: > > Nit: swapInNewVisitor => setVisitor ? > > (Swap sounds like it swaps the two visitors.) Done.
https://codereview.chromium.org/2625093002/diff/90001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp (right): https://codereview.chromium.org/2625093002/diff/90001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:175: m_nodesRequiringTracing[root].push_back(node); Why don't you need to push Nodes that are found during tracePendingActivities? i.e., why do you need to push only Nodes that are found during VisitPersistentHandle? https://codereview.chromium.org/2625093002/diff/90001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:191: AbortTracing(); What's a difference between AbortTracing and Epilogue? Does AbortTracing finish the current tracing forcibly? (I was assuming that AbortTracing gives up the ongoing incremental tracing, throwing away various states.) https://codereview.chromium.org/2625093002/diff/90001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:200: void traceNodesThatRequireTracing() { I'm not sure why we need the complexity of traceNodesThatRequireTracing. Would it be possible to directly call findV8WrappersDirectlyReachableFrom() when you find a Node object during VisitPersistentHandle? Then we won't need m_nodesRequiringTracing. https://codereview.chromium.org/2625093002/diff/90001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:216: if (!m_tracingInProgress) Can this happen? https://codereview.chromium.org/2625093002/diff/90001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:228: } Don't we need to reset m_firstTracedScriptWrappable to true after calling traceWrappers? Otherwise, findV8WrappersDirectlyReachableFrom will always return only one ScriptWrappable, right? https://codereview.chromium.org/2625093002/diff/90001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:236: m_firstTracedScriptWrappable = true; Maybe should we flip true/false? Initially the first ScriptWrappable is NOT yet traced. https://codereview.chromium.org/2625093002/diff/90001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:259: mutable v8::HeapProfiler::RetainerChildren m_tempFoundV8Wrappers; m_tempFoundV8Wrappers => m_foundV8Wrappers https://codereview.chromium.org/2625093002/diff/90001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:274: HeapSnaphotWrapperVisitor tracer(isolate); I guess we should use: std::unique_ptr<HeapSnaphotWrapperVisitor> tracer = new ...; See my comment in V8PerIsolateData. https://codereview.chromium.org/2625093002/diff/90001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:279: isolate->VisitHandlesWithClassIds(&tracer); Nit: I'd prefer writing this as tracer->traceV8Roots(). https://codereview.chromium.org/2625093002/diff/90001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:282: tracer.traceNodesThatRequireTracing(); Nit: tracer->traceBlinkRoots(). https://codereview.chromium.org/2625093002/diff/90001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.cpp (right): https://codereview.chromium.org/2625093002/diff/90001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.cpp:404: current->performCleanup(); Would you elaborate on why it's okay to call performCleanup()? Isn't it possible that the current visitor is popped again later and tries to continue the incremental tracing? https://codereview.chromium.org/2625093002/diff/90001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.cpp:405: V8PerIsolateData::from(m_isolate)->m_scriptWrappableVisitor.release(); Ah, you need .release() because you need to leak the pointer -- which looks a bit nasty. Can we make TemporaryScriptWrappableVisitorScope::m_savedVisitor a std::unique_ptr<ScriptWrappableVisitor>? Then you can just pass an ownership to m_savedVisitor instead of leaking.
Thanks, addressed comments and answered questions. https://codereview.chromium.org/2625093002/diff/90001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp (right): https://codereview.chromium.org/2625093002/diff/90001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:175: m_nodesRequiringTracing[root].push_back(node); On 2017/01/16 02:46:56, haraken wrote: > > Why don't you need to push Nodes that are found during tracePendingActivities? > i.e., why do you need to push only Nodes that are found during > VisitPersistentHandle? > VisitPersistentHandle visits all persistent handles V8 knows about. If a "Node" would be a pending activity, we would also catch it here. This mirrors the groups that are currently collected for devtools, which basically groups DOM nodes by DOM tree and puts pending activities in a separate group. https://codereview.chromium.org/2625093002/diff/90001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:191: AbortTracing(); On 2017/01/16 02:46:56, haraken wrote: > > What's a difference between AbortTracing and Epilogue? Does AbortTracing finish > the current tracing forcibly? (I was assuming that AbortTracing gives up the > ongoing incremental tracing, throwing away various states.) Since we finished the line before with FORE_COMPLETION, the only difference is that we synchronously do performCleanup(). The is no idle time here, so a asynchronous cleanup using tasks doesn't make sense. We might want to add this possibility to the tracer API, e.g. have TraceEpilogue(EmbedderHeapTracer::SYNCHRONOUS); https://codereview.chromium.org/2625093002/diff/90001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:200: void traceNodesThatRequireTracing() { On 2017/01/16 02:46:56, haraken wrote: > > I'm not sure why we need the complexity of traceNodesThatRequireTracing. Would > it be possible to directly call findV8WrappersDirectlyReachableFrom() when you > find a Node object during VisitPersistentHandle? Then we won't need > m_nodesRequiringTracing. > We cannot do that, as we need a mapping from nodes to RetainedDOMInfo. If we would trace in VisitPersistentHandle, we would need a side map from root->RetainedDOMInfo, as we would need to keep track of that mapping. I think the 2-pass approach is slightly easier to understand. https://codereview.chromium.org/2625093002/diff/90001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:216: if (!m_tracingInProgress) On 2017/01/16 02:46:56, haraken wrote: > > Can this happen? Removed as it cannot happen. This was copied over from the regular tracer. https://codereview.chromium.org/2625093002/diff/90001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:228: } On 2017/01/16 02:46:56, haraken wrote: > > Don't we need to reset m_firstTracedScriptWrappable to true after calling > traceWrappers? > > Otherwise, findV8WrappersDirectlyReachableFrom will always return only one > ScriptWrappable, right? Yes, that's the intention. It should find wrappers reachable from this exact ScriptWrappable. It will trace through non-scriptwrappables (e.g. NodeRareData and friends) though. https://codereview.chromium.org/2625093002/diff/90001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:236: m_firstTracedScriptWrappable = true; On 2017/01/16 02:46:56, haraken wrote: > > Maybe should we flip true/false? > > Initially the first ScriptWrappable is NOT yet traced. Done. https://codereview.chromium.org/2625093002/diff/90001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:259: mutable v8::HeapProfiler::RetainerChildren m_tempFoundV8Wrappers; On 2017/01/16 02:46:56, haraken wrote: > > m_tempFoundV8Wrappers => m_foundV8Wrappers Done. https://codereview.chromium.org/2625093002/diff/90001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:274: HeapSnaphotWrapperVisitor tracer(isolate); On 2017/01/16 02:46:56, haraken wrote: > > I guess we should use: > > std::unique_ptr<HeapSnaphotWrapperVisitor> tracer = new ...; > > See my comment in V8PerIsolateData. Done. https://codereview.chromium.org/2625093002/diff/90001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.cpp (right): https://codereview.chromium.org/2625093002/diff/90001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.cpp:404: current->performCleanup(); On 2017/01/16 02:46:56, haraken wrote: > > Would you elaborate on why it's okay to call performCleanup()? Isn't it possible > that the current visitor is popped again later and tries to continue the > incremental tracing? We can only swap tracers when tracing is not yet started. Not started still could mean that we have a pending cleanup task. performCleanup() conditionally cleans up the tracer. I added a CHECK to performCleanup that we are not in tracing mode. https://codereview.chromium.org/2625093002/diff/90001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.cpp:405: V8PerIsolateData::from(m_isolate)->m_scriptWrappableVisitor.release(); On 2017/01/16 02:46:56, haraken wrote: > > Ah, you need .release() because you need to leak the pointer -- which looks a > bit nasty. > > Can we make TemporaryScriptWrappableVisitorScope::m_savedVisitor a > std::unique_ptr<ScriptWrappableVisitor>? Then you can just pass an ownership to > m_savedVisitor instead of leaking. Ah, now I understand, thanks! Done.
https://codereview.chromium.org/2625093002/diff/90001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp (right): https://codereview.chromium.org/2625093002/diff/90001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:279: isolate->VisitHandlesWithClassIds(&tracer); On 2017/01/16 02:46:56, haraken wrote: > > Nit: I'd prefer writing this as tracer->traceV8Roots(). Done: collectV8Roots. https://codereview.chromium.org/2625093002/diff/90001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:282: tracer.traceNodesThatRequireTracing(); On 2017/01/16 02:46:56, haraken wrote: > > Nit: tracer->traceBlinkRoots(). This is now traceV8Roots, as it traces exactly what we collected above.
LGTM, I like how this ended up looking! Thank you Michael.
Mostly looks good. Do you think that this feature is well tested in the layout tests (which are currently disabled)? https://codereview.chromium.org/2625093002/diff/90001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp (right): https://codereview.chromium.org/2625093002/diff/90001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:175: m_nodesRequiringTracing[root].push_back(node); On 2017/01/16 10:45:06, Michael Lippautz wrote: > On 2017/01/16 02:46:56, haraken wrote: > > > > Why don't you need to push Nodes that are found during tracePendingActivities? > > i.e., why do you need to push only Nodes that are found during > > VisitPersistentHandle? > > > > VisitPersistentHandle visits all persistent handles V8 knows about. If a "Node" > would be a pending activity, we would also catch it here. > > This mirrors the groups that are currently collected for devtools, which > basically groups DOM nodes by DOM tree and puts pending activities in a separate > group. What happens if there is a pending activity on a DOM object X (which is not a Node) and there are Nodes reachable from X? (But I agree with you that this CL keeps the existing behavior.) https://codereview.chromium.org/2625093002/diff/90001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:191: AbortTracing(); On 2017/01/16 10:45:06, Michael Lippautz wrote: > On 2017/01/16 02:46:56, haraken wrote: > > > > What's a difference between AbortTracing and Epilogue? Does AbortTracing > finish > > the current tracing forcibly? (I was assuming that AbortTracing gives up the > > ongoing incremental tracing, throwing away various states.) > > Since we finished the line before with FORE_COMPLETION, the only difference is > that we synchronously do performCleanup(). The is no idle time here, so a > asynchronous cleanup using tasks doesn't make sense. > > We might want to add this possibility to the tracer API, e.g. have > TraceEpilogue(EmbedderHeapTracer::SYNCHRONOUS); Yeah, that looks tidier. (You can address it in a follow-up though.) https://codereview.chromium.org/2625093002/diff/90001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:228: } On 2017/01/16 10:45:06, Michael Lippautz wrote: > On 2017/01/16 02:46:56, haraken wrote: > > > > Don't we need to reset m_firstTracedScriptWrappable to true after calling > > traceWrappers? > > > > Otherwise, findV8WrappersDirectlyReachableFrom will always return only one > > ScriptWrappable, right? > > Yes, that's the intention. It should find wrappers reachable from this exact > ScriptWrappable. > > It will trace through non-scriptwrappables (e.g. NodeRareData and friends) > though. But what happens if there are multiple ScriptWrappables (at the first level) reachable from the Node?
Test coverage is ok as no existing tests are disabled. We currently generate snapshots using object grouping and after round of patches (v8 api, this CL, v8 snapshot generator) we will have switched to using tracing, without ever disabling the tests. https://codereview.chromium.org/2625093002/diff/90001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp (right): https://codereview.chromium.org/2625093002/diff/90001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:175: m_nodesRequiringTracing[root].push_back(node); On 2017/01/16 12:48:13, haraken wrote: > On 2017/01/16 10:45:06, Michael Lippautz wrote: > > On 2017/01/16 02:46:56, haraken wrote: > > > > > > Why don't you need to push Nodes that are found during > tracePendingActivities? > > > i.e., why do you need to push only Nodes that are found during > > > VisitPersistentHandle? > > > > > > > VisitPersistentHandle visits all persistent handles V8 knows about. If a > "Node" > > would be a pending activity, we would also catch it here. > > > > This mirrors the groups that are currently collected for devtools, which > > basically groups DOM nodes by DOM tree and puts pending activities in a > separate > > group. > > What happens if there is a pending activity on a DOM object X (which is not a > Node) and there are Nodes reachable from X? > > (But I agree with you that this CL keeps the existing behavior.) Then tracePendingActivities will catch it. https://codereview.chromium.org/2625093002/diff/90001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:191: AbortTracing(); On 2017/01/16 12:48:12, haraken wrote: > On 2017/01/16 10:45:06, Michael Lippautz wrote: > > On 2017/01/16 02:46:56, haraken wrote: > > > > > > What's a difference between AbortTracing and Epilogue? Does AbortTracing > > finish > > > the current tracing forcibly? (I was assuming that AbortTracing gives up the > > > ongoing incremental tracing, throwing away various states.) > > > > Since we finished the line before with FORE_COMPLETION, the only difference is > > that we synchronously do performCleanup(). The is no idle time here, so a > > asynchronous cleanup using tasks doesn't make sense. > > > > We might want to add this possibility to the tracer API, e.g. have > > TraceEpilogue(EmbedderHeapTracer::SYNCHRONOUS); > > Yeah, that looks tidier. (You can address it in a follow-up though.) Acknowledged. https://codereview.chromium.org/2625093002/diff/90001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:228: } On 2017/01/16 12:48:13, haraken wrote: > On 2017/01/16 10:45:06, Michael Lippautz wrote: > > On 2017/01/16 02:46:56, haraken wrote: > > > > > > Don't we need to reset m_firstTracedScriptWrappable to true after calling > > > traceWrappers? > > > > > > Otherwise, findV8WrappersDirectlyReachableFrom will always return only one > > > ScriptWrappable, right? > > > > Yes, that's the intention. It should find wrappers reachable from this exact > > ScriptWrappable. > > > > It will trace through non-scriptwrappables (e.g. NodeRareData and friends) > > though. > > But what happens if there are multiple ScriptWrappables (at the first level) > reachable from the Node? They are traced and we insert edges from the v8 wrapper represented by the current ScriptWrappable to all v8 wrappers of the children ScriptWrappables. The details: - We add m_currentParent (=ScriptWrappable) to the deque through the call traceable->wrapperTypeInfo()->traceWrappers(this, traceable) on line 241. - Call to AdvanceTracing. There's only a single object on the marking deque. - m_firstTracedScriptWrappableTraced is false, so we can trace this object. - Tracing enqueues all children (ScriptWrappables, and special cases) onto the deque. - m_firstTracedScriptWrappableTraced is set to true, so only special cases will be further traced (effectively hiding the intermediate links from the snapshot) - All found V8 wrappers from this one level of tracing will be assigned an edge starting at the initial wrapper.
LGTM with comments. https://codereview.chromium.org/2625093002/diff/90001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp (right): https://codereview.chromium.org/2625093002/diff/90001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:228: } On 2017/01/16 13:15:38, Michael Lippautz wrote: > On 2017/01/16 12:48:13, haraken wrote: > > On 2017/01/16 10:45:06, Michael Lippautz wrote: > > > On 2017/01/16 02:46:56, haraken wrote: > > > > > > > > Don't we need to reset m_firstTracedScriptWrappable to true after calling > > > > traceWrappers? > > > > > > > > Otherwise, findV8WrappersDirectlyReachableFrom will always return only one > > > > ScriptWrappable, right? > > > > > > Yes, that's the intention. It should find wrappers reachable from this exact > > > ScriptWrappable. > > > > > > It will trace through non-scriptwrappables (e.g. NodeRareData and friends) > > > though. > > > > But what happens if there are multiple ScriptWrappables (at the first level) > > reachable from the Node? > > They are traced and we insert edges from the v8 wrapper represented by the > current ScriptWrappable to all v8 wrappers of the children ScriptWrappables. > > The details: > - We add m_currentParent (=ScriptWrappable) to the deque through the call > traceable->wrapperTypeInfo()->traceWrappers(this, traceable) on line 241. > - Call to AdvanceTracing. There's only a single object on the marking deque. > - m_firstTracedScriptWrappableTraced is false, so we can trace this object. > - Tracing enqueues all children (ScriptWrappables, and special cases) onto the > deque. > - m_firstTracedScriptWrappableTraced is set to true, so only special cases will > be further traced (effectively hiding the intermediate links from the snapshot) > - All found V8 wrappers from this one level of tracing will be assigned an edge > starting at the initial wrapper. Imagine the following object graph: Node --> TraceWrappable1 --> ScriptWrappable1 | ----> TraceWrappable2 --> TraceWrappable3 --> ScriptWrappable2 In this case, ScriptWrappable1 will be added to m_foundV8Wrappers but ScriptWrappable2 won't be added, right? (because m_onlyTraceSingleLevel prevents us from tracing from TraceWrappable3 to ScriptWrappable2) https://codereview.chromium.org/2625093002/diff/90001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.cpp (right): https://codereview.chromium.org/2625093002/diff/90001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.cpp:404: current->performCleanup(); On 2017/01/16 10:45:06, Michael Lippautz wrote: > On 2017/01/16 02:46:56, haraken wrote: > > > > Would you elaborate on why it's okay to call performCleanup()? Isn't it > possible > > that the current visitor is popped again later and tries to continue the > > incremental tracing? > > We can only swap tracers when tracing is not yet started. Not started still > could mean that we have a pending cleanup task. performCleanup() conditionally > cleans up the tracer. > > I added a CHECK to performCleanup that we are not in tracing mode. How is it guaranteed that V8GCController::getRetainerInfos is called when the tracing is not yet started? Does V8 guarantee the fact? https://codereview.chromium.org/2625093002/diff/110001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp (right): https://codereview.chromium.org/2625093002/diff/110001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:160: m_firstTracedScriptWrappableTraced(false) { m_firstTracedScriptWrappableTraced => m_firstScriptWrappableTraced https://codereview.chromium.org/2625093002/diff/110001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:161: m_nodesRequiringTracing.clear(); Remove this.
https://codereview.chromium.org/2625093002/diff/90001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp (right): https://codereview.chromium.org/2625093002/diff/90001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:228: } On 2017/01/17 00:31:25, haraken wrote: > On 2017/01/16 13:15:38, Michael Lippautz wrote: > > On 2017/01/16 12:48:13, haraken wrote: > > > On 2017/01/16 10:45:06, Michael Lippautz wrote: > > > > On 2017/01/16 02:46:56, haraken wrote: > > > > > > > > > > Don't we need to reset m_firstTracedScriptWrappable to true after > calling > > > > > traceWrappers? > > > > > > > > > > Otherwise, findV8WrappersDirectlyReachableFrom will always return only > one > > > > > ScriptWrappable, right? > > > > > > > > Yes, that's the intention. It should find wrappers reachable from this > exact > > > > ScriptWrappable. > > > > > > > > It will trace through non-scriptwrappables (e.g. NodeRareData and friends) > > > > though. > > > > > > But what happens if there are multiple ScriptWrappables (at the first level) > > > reachable from the Node? > > > > They are traced and we insert edges from the v8 wrapper represented by the > > current ScriptWrappable to all v8 wrappers of the children ScriptWrappables. > > > > The details: > > - We add m_currentParent (=ScriptWrappable) to the deque through the call > > traceable->wrapperTypeInfo()->traceWrappers(this, traceable) on line 241. > > - Call to AdvanceTracing. There's only a single object on the marking deque. > > - m_firstTracedScriptWrappableTraced is false, so we can trace this object. > > - Tracing enqueues all children (ScriptWrappables, and special cases) onto the > > deque. > > - m_firstTracedScriptWrappableTraced is set to true, so only special cases > will > > be further traced (effectively hiding the intermediate links from the > snapshot) > > - All found V8 wrappers from this one level of tracing will be assigned an > edge > > starting at the initial wrapper. > > Imagine the following object graph: > > Node --> TraceWrappable1 --> ScriptWrappable1 > | > ----> TraceWrappable2 --> TraceWrappable3 --> ScriptWrappable2 > > In this case, ScriptWrappable1 will be added to m_foundV8Wrappers but > ScriptWrappable2 won't be added, right? (because m_onlyTraceSingleLevel prevents > us from tracing from TraceWrappable3 to ScriptWrappable2) Works, and here is why :) My assumptions: - Node is also ScriptWrappable - TraceWrappable 1-3 are TraceWrapperBase - ScriptWrappable 1+2 are ScriptWrappable The key is that we always trace through non-ScriptWrappables and establish edges to their children. What happens in details: - Marking Deque (MD): <||> (empty) - We will dispatchTraceWrappers for Node on line 241 (traceable->wrapperTypeInfo()->traceWrappers(this, traceable)). - After Node m_firstScriptWrappableTraced will be true and the marking deque (MD) will be <|TraceWrappable1|TraceWrappable2|> - We then traceWrappers for TraceWrappable1. This will work since it's not a ScriptWrappable. After this one MD: <|TraceWrappable2|ScriptWrappabel1|> - Same for TraceWrappable2. MD: <|ScriptWrappable1|TraceWrappable3|>. - When we try to process ScriptWrappable1, we will not trace anymore, but do markWrappersInAllWorls, so we find the edge. MD: <|TraceWrappable3|>. - TraceWrappable3 is not a ScriptWrappable, so we just trace. MD: <|ScriptWrappable2|> - We don't trace ScriptWrappable2, but add the edge because we find its wrapper through markWrappersInAllWorlds. Let me know if this is not clear! https://codereview.chromium.org/2625093002/diff/90001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.cpp (right): https://codereview.chromium.org/2625093002/diff/90001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.cpp:404: current->performCleanup(); On 2017/01/17 00:31:25, haraken wrote: > On 2017/01/16 10:45:06, Michael Lippautz wrote: > > On 2017/01/16 02:46:56, haraken wrote: > > > > > > Would you elaborate on why it's okay to call performCleanup()? Isn't it > > possible > > > that the current visitor is popped again later and tries to continue the > > > incremental tracing? > > > > We can only swap tracers when tracing is not yet started. Not started still > > could mean that we have a pending cleanup task. performCleanup() conditionally > > cleans up the tracer. > > > > I added a CHECK to performCleanup that we are not in tracing mode. > > How is it guaranteed that V8GCController::getRetainerInfos is called when the > tracing is not yet started? Does V8 guarantee the fact? > Yes, V8 guarantees that. https://codereview.chromium.org/2625093002/diff/110001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp (right): https://codereview.chromium.org/2625093002/diff/110001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:160: m_firstTracedScriptWrappableTraced(false) { On 2017/01/17 00:31:25, haraken wrote: > > m_firstTracedScriptWrappableTraced => m_firstScriptWrappableTraced Done. https://codereview.chromium.org/2625093002/diff/110001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:161: m_nodesRequiringTracing.clear(); On 2017/01/17 00:31:26, haraken wrote: > > Remove this. Whoops, done.
LGTM! https://codereview.chromium.org/2625093002/diff/90001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp (right): https://codereview.chromium.org/2625093002/diff/90001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:228: } On 2017/01/17 09:15:32, Michael Lippautz wrote: > On 2017/01/17 00:31:25, haraken wrote: > > On 2017/01/16 13:15:38, Michael Lippautz wrote: > > > On 2017/01/16 12:48:13, haraken wrote: > > > > On 2017/01/16 10:45:06, Michael Lippautz wrote: > > > > > On 2017/01/16 02:46:56, haraken wrote: > > > > > > > > > > > > Don't we need to reset m_firstTracedScriptWrappable to true after > > calling > > > > > > traceWrappers? > > > > > > > > > > > > Otherwise, findV8WrappersDirectlyReachableFrom will always return only > > one > > > > > > ScriptWrappable, right? > > > > > > > > > > Yes, that's the intention. It should find wrappers reachable from this > > exact > > > > > ScriptWrappable. > > > > > > > > > > It will trace through non-scriptwrappables (e.g. NodeRareData and > friends) > > > > > though. > > > > > > > > But what happens if there are multiple ScriptWrappables (at the first > level) > > > > reachable from the Node? > > > > > > They are traced and we insert edges from the v8 wrapper represented by the > > > current ScriptWrappable to all v8 wrappers of the children ScriptWrappables. > > > > > > The details: > > > - We add m_currentParent (=ScriptWrappable) to the deque through the call > > > traceable->wrapperTypeInfo()->traceWrappers(this, traceable) on line 241. > > > - Call to AdvanceTracing. There's only a single object on the marking deque. > > > - m_firstTracedScriptWrappableTraced is false, so we can trace this object. > > > - Tracing enqueues all children (ScriptWrappables, and special cases) onto > the > > > deque. > > > - m_firstTracedScriptWrappableTraced is set to true, so only special cases > > will > > > be further traced (effectively hiding the intermediate links from the > > snapshot) > > > - All found V8 wrappers from this one level of tracing will be assigned an > > edge > > > starting at the initial wrapper. > > > > Imagine the following object graph: > > > > Node --> TraceWrappable1 --> ScriptWrappable1 > > | > > ----> TraceWrappable2 --> TraceWrappable3 --> ScriptWrappable2 > > > > In this case, ScriptWrappable1 will be added to m_foundV8Wrappers but > > ScriptWrappable2 won't be added, right? (because m_onlyTraceSingleLevel > prevents > > us from tracing from TraceWrappable3 to ScriptWrappable2) > > Works, and here is why :) > > My assumptions: > - Node is also ScriptWrappable > - TraceWrappable 1-3 are TraceWrapperBase > - ScriptWrappable 1+2 are ScriptWrappable > > The key is that we always trace through non-ScriptWrappables and establish edges > to their children. Ah, I was missing this point. > > What happens in details: > - Marking Deque (MD): <||> (empty) > - We will dispatchTraceWrappers for Node on line 241 > (traceable->wrapperTypeInfo()->traceWrappers(this, traceable)). > - After Node m_firstScriptWrappableTraced will be true and the marking deque > (MD) will be <|TraceWrappable1|TraceWrappable2|> > - We then traceWrappers for TraceWrappable1. This will work since it's not a > ScriptWrappable. After this one MD: <|TraceWrappable2|ScriptWrappabel1|> > - Same for TraceWrappable2. MD: <|ScriptWrappable1|TraceWrappable3|>. > - When we try to process ScriptWrappable1, we will not trace anymore, but do > markWrappersInAllWorls, so we find the edge. MD: <|TraceWrappable3|>. > - TraceWrappable3 is not a ScriptWrappable, so we just trace. MD: > <|ScriptWrappable2|> > - We don't trace ScriptWrappable2, but add the edge because we find its wrapper > through markWrappersInAllWorlds. > > Let me know if this is not clear! > >
Alright, due to API changes this will all happen after the branch cut, thanks.
Description was changed from ========== [wrapper-tracing] Add heap snapshot generator infrastructure Register a callback with V8's heap profiler that can be used to query all RetainedObjectInfo_s for a given Isolate. Basically, we create RetainedObjectInfo_s for DOM trees (attached and detached) and pending activities and assign all found wrappers to these info objects. BUG=679724 ========== to ========== [wrapper-tracing] Add heap snapshot generator infrastructure Register a callback with V8's heap profiler that can be used to query all RetainedObjectInfo_s for a given Isolate. Basically, we create RetainedObjectInfo_s for DOM trees (attached and detached) and pending activities and assign all found wrappers to these info objects. BUG=chromium:679724 ==========
The CQ bit was checked by mlippautz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by mlippautz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by mlippautz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mlippautz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, alph@chromium.org, hlopko@chromium.org, haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2625093002/#ps170001 (title: "Rebase and fix CHECK")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 170001, "attempt_start_ts": 1485177567636000,
"parent_rev": "c2cdce4467fb89b77f62b3b042b5cdfd19739f13", "commit_rev":
"a34d185b3e31187fb8aa16b700e5837e9636cba9"}
Message was sent while issue was closed.
Description was changed from ========== [wrapper-tracing] Add heap snapshot generator infrastructure Register a callback with V8's heap profiler that can be used to query all RetainedObjectInfo_s for a given Isolate. Basically, we create RetainedObjectInfo_s for DOM trees (attached and detached) and pending activities and assign all found wrappers to these info objects. BUG=chromium:679724 ========== to ========== [wrapper-tracing] Add heap snapshot generator infrastructure Register a callback with V8's heap profiler that can be used to query all RetainedObjectInfo_s for a given Isolate. Basically, we create RetainedObjectInfo_s for DOM trees (attached and detached) and pending activities and assign all found wrappers to these info objects. BUG=chromium:679724 Review-Url: https://codereview.chromium.org/2625093002 Cr-Commit-Position: refs/heads/master@{#445372} Committed: https://chromium.googlesource.com/chromium/src/+/a34d185b3e31187fb8aa16b700e5... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:170001) as https://chromium.googlesource.com/chromium/src/+/a34d185b3e31187fb8aa16b700e5... |
