[bindings] Implement ScriptWrappable::TraceWrappers. [chromium/src : master]
Ulan Degenbaev (Gerrit)
Michael Lippautz (Gerrit)
lgtm, it's nicely shaping up now
Please also wait for Kentaro's comment on ScriptWrappable::TraceWrappers.
Patch set 3:Code-Review +1
4 comments:
File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:
\o/ awesome that this is handled through the regular interface.
File third_party/WebKit/Source/platform/bindings/ScriptWrappable.cpp:
Patch Set #3, Line 44:
TraceWrappersI would be fine with landing this one already. The GC plugin is on the way (needs clang roll) and I already fixed all the issues it found.
Kentaro, wdyt?
File third_party/WebKit/Source/platform/bindings/ScriptWrappableVisitor.h:
File third_party/WebKit/Source/platform/heap/TraceTraits.h:
Patch Set #3, Line 111:
TraceMarkedWrapperThis should probably just be TraceWrappers but then it does not fit nicely into AdjustAnd*Mark*. Lets see what this looks like when we apply the same approach to Oilpan.
Kentaro Hara (Gerrit)
4 comments:
File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:
Patch Set #3, Line 219:
// main thread wrapper as we cannot properly record DOMWrapperMap values.main thread => main world
File third_party/WebKit/Source/platform/bindings/DOMDataStore.h:
Patch Set #3, Line 146:
void TraceWrappers(const ScriptWrappable* script_wrappable,Can we make DOMDataStore inherit from TraceWrapperBase (instead of creating manually dispatched TraceWrappers)?
File third_party/WebKit/Source/platform/bindings/DOMWrapperWorld.h:
Patch Set #3, Line 98:
static void TraceWrappers(const ScriptWrappable*,Instead of making this a static method, can we make DOMWrapperMap inherit from TraceWrapperBase and use TraceWrapperBase::TraceWrappers?
ScriptWrappable::TraceWrappers can iterate all wrapper maps and call visitor->TraceWrappers(wrapper_map).
If we do this, can we probably remove Visit(DOMWrapperMap<ScriptWrappable>*, ScriptWrappable*)? (I'm not fully convinced why we need a special Visit method for DOMWrapperMap.)
File third_party/WebKit/Source/platform/bindings/ScriptWrappable.cpp:
Patch Set #3, Line 44:
TraceWrappers
I would be fine with landing this one already. […]
Sounds reasonable since it will take a few weeks to roll the clang plugin.
Just help me understand: Before Ulan started this refactoring and Michael fixed the parent class tracing, how was main_world_wrapper_ traced?
To view, visit change 874190. To unsubscribe, or for help writing mail filters, visit settings.
Michael Lippautz (Gerrit)
1 comment:
Patch Set #3, Line 44:
TraceWrappers
Sounds reasonable since it will take a few weeks to roll the clang plugin. […]
It was traced through ScriptWrappableVisitor::MarkWrappersInAllWorlds [1]. This one dispatched to the DOMWrapperWorld [2] which then also marked then passed the main world wrapper back to the visitor and passed the other ones directly to V8.
The problem here was that (a) ScriptWrappableVisitor hat internal knowledge on ScriptWrappable., (b) the DOMWrapper world bypassed the visitor for all but the main world.
In the new approach, ScriptWrappable is no different then other objects which allows us to remove a bunch of special casing.
[1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/bindings/ScriptWrappableVisitor.cpp?q=MarkWrappersInAllWorlds&l=219
[2] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/bindings/DOMWrapperWorld.cpp?l=119
To view, visit change 874190. To unsubscribe, or for help writing mail filters, visit settings.
Kentaro Hara (Gerrit)
1 comment:
Patch Set #3, Line 44:
TraceWrappers
It was traced through ScriptWrappableVisitor::MarkWrappersInAllWorlds [1]. […]
Ah, thanks! Makes sense.
To view, visit change 874190. To unsubscribe, or for help writing mail filters, visit settings.
Ulan Degenbaev (Gerrit)
2 comments:
Patch Set #3, Line 146:
void TraceWrappers(const ScriptWrappable* script_wrappable,
Can we make DOMDataStore inherit from TraceWrapperBase (instead of creating manually dispatched Trac […]
If DOMDataStore inherits from TraceWrapperBase, then it gets its own mark bit, right? i.e. it becomes a real node in GC graph.
Marking DOMDataStore would mark all wrappers, which will cause memory leaks (see my reply in the other comment).
File third_party/WebKit/Source/platform/bindings/DOMWrapperWorld.h:
Patch Set #3, Line 98:
static void TraceWrappers(const ScriptWrappable*,
Instead of making this a static method, can we make DOMWrapperMap inherit from TraceWrapperBase and […]
visitor->TraceWrappers(wrapper_map) would put wrapper_map into marking deque.
Later when we get the wrapper_map from the marking deque and process it, we will have to mark all wrappers in the map, which is not what we want.We want to mark only the wrappers corresponding to this script wrappable.
To view, visit change 874190. To unsubscribe, or for help writing mail filters, visit settings.
Ulan Degenbaev (Gerrit)
1 comment:
File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:
Patch Set #3, Line 219:
// main world wrapper as we cannot properly record DOMWrapperMap values.
main thread => main world
Done
To view, visit change 874190. To unsubscribe, or for help writing mail filters, visit settings.
Kentaro Hara (Gerrit)
1 comment:
Patch Set #3, Line 98:
static void TraceWrappers(const ScriptWrappable*,
visitor->TraceWrappers(wrapper_map) would put wrapper_map into marking deque. […]
I got the situation.
Then would it be an option to write it like this?
void ScriptWrappable::TraceWrappers(Visitor* visitor) {
for (auto& wrapper_map : AllWorldsInCurrentThread()) {
visitor->TraceWrappers(wrapper_map->DOMDataStore().Get());
}
}My point is that we won't want to add a special TraceWrappers() method every time we want to trace a not-GarbageCollected class.
To view, visit change 874190. To unsubscribe, or for help writing mail filters, visit settings.
Ulan Degenbaev (Gerrit)
1 comment:
Patch Set #3, Line 98:
static void TraceWrappers(const ScriptWrappable*,
I got the situation. […]
Is the name "TraceWrappers" problematic? If so I can rename it to something different like "DispatchTraceWrappers".
I would like to avoid ScriptWrappable knowing internals of DOMWrapperWorld, DOMDataStore, DOMWrapperMap. Without helper methods on these classes. The code is going to look like:
void ScriptWrappable::TraceWrappers(Visitor* visitor) {
for (DOMWrapperWorld* world : AllWorldsInCurrentThread()) {
DOMDataStore& data_store = world->DomDataStore();
if (data_store.ContainsWrapper(this))
visitor->TraceWrappers(&data_store.wrapper_map().value(), this);
}
}So the ScriptWrappable is going to know that DOMWrapperMap is part of DOMDataStore, which is part of DOMWrapperWorld.
To view, visit change 874190. To unsubscribe, or for help writing mail filters, visit settings.
Kentaro Hara (Gerrit)
1 comment:
Patch Set #3, Line 98:
static void TraceWrappers(const ScriptWrappable*,
Is the name "TraceWrappers" problematic? If so I can rename it to something different like "Dispatch […]
Then can we add a helper like DOMWrapperWorld::GetWrapper() so that ScriptWrappable can get a wrapper without knowing DOMDataStore etc?
We're already using the pattern in some places.
void TraceWrappers(Visitor* visitor) {
visitor->TraceWrappers(GetSomePointer());
}
To view, visit change 874190. To unsubscribe, or for help writing mail filters, visit settings.
Michael Lippautz (Gerrit)
1 comment:
Patch Set #3, Line 98:
static void TraceWrappers(const ScriptWrappable*,
Then can we add a helper like DOMWrapperWorld::GetWrapper() so that ScriptWrappable can get a wrappe […]
TraceWrappers should ideally always delegate to the component owning the pointers. In this case that is the worlds/storage.
We should not reach into other classes objects. (We sometimes did that because it was not possible for other reasons back then but we should really try to keep abstractions as clean as possible.)
To view, visit change 874190. To unsubscribe, or for help writing mail filters, visit settings.
Ulan Degenbaev (Gerrit)
1 comment:
Patch Set #3, Line 98:
static void TraceWrappers(const ScriptWrappable*,
TraceWrappers should ideally always delegate to the component owning the pointers. […]
1) DOMWrapperWorld::GetWrapper() is more generic than TraceWrappers (DispatchTraceWrappes). So we will have to support cases when GetWrapper is called by other components and properly manage lifetime of the returned Wrapper.
2) More importantly, what would be the type of DOMWrapperWorld::GetWrapper()? It is not v8::PersistentBase and not TraceWrapperV8Reference. It is the value part of v8::GlobalValueMap<KeyType*, v8::Object, PersistentValueMapTraits>. I think this where where all this complexity is coming from.In any case, I still don't understand the problem with the current approach in the CL?
DOMWrapperMap, DOMDataStore, and DOMWrapperWorld are not GarbageCollected, but they are already implicitly participating in tracing via MarkWrappersInAllWorlds, MarkWrapper methods.
This CL is only making the participation explicit, which I think is good.
To view, visit change 874190. To unsubscribe, or for help writing mail filters, visit settings.
Kentaro Hara (Gerrit)
1 comment:
Patch Set #3, Line 98:
static void TraceWrappers(const ScriptWrappable*,
1) DOMWrapperWorld::GetWrapper() is more generic than TraceWrappers (DispatchTraceWrappes). […]
The problem is that we're introducing multiple ways to trace into non-TraceWrapperBase objects (yeah, I understand that you're not introducing a new way because you're just replacing MarkWrappersInAllWorlds etc with DispatchTraceWrappers).
Michael: Are we planning to consistently use DispatchTraceWrappers to trace into non-TraceWrapperBase objects? Then we will need to define a special Visit() for all the cases. It will be hard in some cases because ScriptWrappableVisitor is in platform/ but objects are in core/...
In other words, what should be a pattern to trace into non-TraceWrapperBase objects?
To view, visit change 874190. To unsubscribe, or for help writing mail filters, visit settings.
Michael Lippautz (Gerrit)
1 comment:
Patch Set #3, Line 98:
static void TraceWrappers(const ScriptWrappable*,
The problem is that we're introducing multiple ways to trace into non-TraceWrapperBase objects (yeah […]
I see your point now :)
DOMWrapperWorld is not integrated into tracing and we are not calling DispatchTraceWrappers for it. We are merely calling a static function to delegate tracing its implementation details.
This should always be an exception case as we need a manual write barrier. In this case I don't see a way to work around that as it is a data structure on the API boundary (storing raw v8::Object*).
Nothing changes for the regular architecture: We should always inherit from TraceWrapperBase if SWV should be able to trace it. The object types that we need are: (1) TraceWrapperBase, (2) TraceWrapperV8Reference and (3) the world for raw v8::Objects.
Case (3) was currently hidden because we took a look of hacky short curts. Ideally (2) and (3) would merge to a common type. This would require changing V8's API though. E.g. one could image having a DOMWrapperStore that somehow stores V8TraceWrapperReference internally. This would introduce quite some churn on both codebases though.
To view, visit change 874190. To unsubscribe, or for help writing mail filters, visit settings.
Ulan Degenbaev (Gerrit)
Patch Set 4:
(1 comment)
1 comment:
Patch Set #3, Line 98:
static void TraceWrappers(const ScriptWrappable*,
I see your point now :) […]
Addition to Michael reply:
I don't expect special Visit method for other cases.
This case is special because v8::GlobalValueMap<KeyType*, v8::Object, PersistentValueMapTraits> is not convertible to TraceWrapperV8Reference. You can see that it's special because it has its own RegisterExternallyReferencedObject (https://cs.chromium.org/chromium/src/v8/include/v8-util.h?rcl=7199784766ca3b3926eefc39ab7d748e954acc24&l=213)
All other case should be able to use the existing Visit methods.
To view, visit change 874190. To unsubscribe, or for help writing mail filters, visit settings.
Kentaro Hara (Gerrit)
1 comment:
Patch Set #3, Line 98:
static void TraceWrappers(const ScriptWrappable*,
Addition to Michael reply: […]
Thanks, I'm now convinced :)
Will take a final look shortly.
To view, visit change 874190. To unsubscribe, or for help writing mail filters, visit settings.
Kentaro Hara (Gerrit)
LGTM. Thanks for being persistent!
Patch set 4:Code-Review +1
Ulan Degenbaev (Gerrit)
Patch Set 4: Code-Review+1
LGTM. Thanks for being persistent!
Thank you! Can I land 873919? (this CL depends on that).
Ulan Degenbaev (Gerrit)
Patch set 4:Commit-Queue +2
Commit Bot (Gerrit)
CQ is trying the patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"Rebase" https://chromium-review.googlesource.com/c/874190/5
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/874190/5
Bot data: {"action": "start", "triggered_at": "2018-01-20T10:21:31.0Z", "cq_cfg_revision": "a668b5363cd374a29ca0f46124c226e2e2aa21d9", "revision": "4020ad0bdd112ac6c62aa505ed27dae57a0a3823"}
Commit Bot (Gerrit)
Commit Bot merged this change.
[bindings] Implement ScriptWrappable::TraceWrappers.
The function traces wrapper objects corresponding to the
ScriptWrappable in all worlds. This allows us to remove
ad-hoc MarkWrappersInAllWorlds and simplify DispatchTraceWrappers.
Now wrappers in non-main world are properly reported to the visitor.
So the derived visitors (verifier, heap-snapshot generator) can see
and handle these wrappers.
Bug: 802273
Change-Id: Ie5aef1de7f0dcffe3a71304e0a37686d2bc859d3
Reviewed-on: https://chromium-review.googlesource.com/874190
Reviewed-by: Kentaro Hara <har...@chromium.org>
Reviewed-by: Michael Lippautz <mlip...@chromium.org>
Commit-Queue: Ulan Degenbaev <ul...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#530771}
---
M third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp
M third_party/WebKit/Source/platform/bindings/DOMDataStore.h
M third_party/WebKit/Source/platform/bindings/DOMWrapperMap.h
M third_party/WebKit/Source/platform/bindings/DOMWrapperWorld.cpp
M third_party/WebKit/Source/platform/bindings/DOMWrapperWorld.h
M third_party/WebKit/Source/platform/bindings/ScriptWrappable.cpp
M third_party/WebKit/Source/platform/bindings/ScriptWrappable.h
M third_party/WebKit/Source/platform/bindings/ScriptWrappableVisitor.cpp
M third_party/WebKit/Source/platform/bindings/ScriptWrappableVisitor.h
M third_party/WebKit/Source/platform/bindings/ScriptWrappableVisitorVerifier.h
M third_party/WebKit/Source/platform/heap/TraceTraits.h
11 files changed, 44 insertions(+), 69 deletions(-)