Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1876383003:
Introduce infrastructure for tracing ScriptWrappables. (Closed)
|
Created:
4 years, 8 months ago by Marcel Hlopko Modified:
4 years, 8 months ago Reviewers:
haraken, Hannes Payer (out of office), Michael Hablich, jochen (gone - plz use gerrit), Michael Achenbach CC:
Mads Ager (chromium), blink-reviews, blink-reviews-bindings_chromium.org, blink-reviews-dom_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, kinuko+watch, kouhei+heap_chromium.org, oilpan-reviews, rwlbuis, sof Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce infrastructure for tracing ScriptWrappables.
This cl introduces components needed to trace ScriptWrappable references.
Unsurprisingly, this cl does not aim for correctness and is far from tracing all
the wrappables. Instead it demonstrates the approach we've chosen, and will
foster the discussion about future direction. Once we polish the details, I will
upload cls which will improve the tracing coverage.
We introduced TraceWrappableReferences idl annotation, which will generate the
tracing method. The intended usage is demonstrated on Node class. Not all
objects we have to trace through are ScriptWrappable instances, and the intended
solution to this problem is shown on NodeRareData class.
Comments are more than welcome!
BUG=468240
LOG=no
Committed: https://crrev.com/47fcf60157c8e5e4898e8fc4eeafbe5a92159589
Cr-Commit-Position: refs/heads/master@{#388506}
Patch Set 1 #Patch Set 2 : Rebase master #
Total comments: 4
Patch Set 3 : Incorporate Haraken's wonderful comments #
Total comments: 42
Patch Set 4 : Incorporate Haraken's wonderful comments #Patch Set 5 : Fix comment #Patch Set 6 : Revert to traceActiveScriptWrappables - C++ is happier #
Total comments: 41
Patch Set 7 : Incorporate Haraken's wonderful comments #Patch Set 8 : Use new v8 api #Patch Set 9 : Use newer v8 api #
Total comments: 38
Patch Set 10 : Addressing final comments #Patch Set 11 : Addressing comments #Patch Set 12 : Final polish :) #Patch Set 13 : Fix var used only in assert #Messages
Total messages: 64 (11 generated)
hlopko@chromium.org changed reviewers: + haraken@chromium.org, hpayer@chromium.org, jochen@chromium.org
Ptal.
Haraken:
>
> I'm not quite sure if it's a good idea to generate the tracing code. I was
thinking that the tracing code will anyway require a lot of customized logic, so
it would be simpler to just ask people to hand-write the contents of the tracing
method (rather than asking people to write const getters required by the
auto-generated tracing code) like this:
>
> void Node::traceWrapper(ScriptWrappableVisitor* visitor) {
> visitor->trace(m_firstChild);
> visitor->trace(m_parentNode);
> ...;
> }
>
> But I'm not sure :)
>
I started with writing all the methods by hand, but soon it turned out its a
mechanical process easily replaceable with code generation. Even the more
complex cases such as Node or Document turned out to be "generatable", after a
slight refactoring (https://codereview.chromium.org/1876843002). Of course, in
case when the method is too specific, the programmer can write it by hand. Wdyt?
On 2016/04/12 11:52:01, Marcel Hlopko wrote:
> Haraken:
> >
> > I'm not quite sure if it's a good idea to generate the tracing code. I was
> thinking that the tracing code will anyway require a lot of customized logic,
so
> it would be simpler to just ask people to hand-write the contents of the
tracing
> method (rather than asking people to write const getters required by the
> auto-generated tracing code) like this:
> >
> > void Node::traceWrapper(ScriptWrappableVisitor* visitor) {
> > visitor->trace(m_firstChild);
> > visitor->trace(m_parentNode);
> > ...;
> > }
> >
> > But I'm not sure :)
> >
>
> I started with writing all the methods by hand, but soon it turned out its a
> mechanical process easily replaceable with code generation. Even the more
> complex cases such as Node or Document turned out to be "generatable", after a
> slight refactoring (https://codereview.chromium.org/1876843002). Of course, in
> case when the method is too specific, the programmer can write it by hand.
Wdyt?
Thanks for the CL! Let me take a close look tomorrow morning.
https://codereview.chromium.org/1876383003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h (right): https://codereview.chromium.org/1876383003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h:56: class ScriptWrappableVisitor { It's a bit confusing to mix "trace", "visit", "accept" and "mark". Also it would be great if we could unify the terms with Oilpan. Oilpan is using "trace" when visiting an object and "mark" when marking an object. https://codereview.chromium.org/1876383003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Node.idl (right): https://codereview.chromium.org/1876383003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.idl:25: TraceWrappableReferences=(ownerDocument,parentNode,parentElement,nextSibling,previousSibling,firstChild,maybeRareData) Hmm. In your approach, developers need to do the following things: 1) Write a list of methods to be traced by the ScriptWrappableVisitor in an IDL file. 2) If the method is not an existing const getter, implement the getter. 3) Add ScriptWrappableVisitor::accept(const T* data) as needed. I'm a bit concerned about 2) because in long term we're planning to replace almost all [SetWrapperReferenceFrom/To], ScriptValues, ScopedPersistents in the code base with the traceWrapper. Then we'll end up with implementing a lot of getters. Alternately I think it would be more straightforward to ask developers to write traceWrapper methods in each C++ class. Also it is aligned with the tracing model of Oilpan. class Node { void traceWrapper(ScriptWrappableVisitor* visitor) { visitor->trace(m_parent); visitor->trace(m_firstChild); visitor->trace(m_nodeRareData); } }; Also I'd like to keep the embedder-side tracing APIs as simple as possible. For example, I want to remove the distinction between "trace", "mark", "visit" and "accept", and the distinction between "traceWrapper" and "traceWrapperChildren". We can do this by: - making visitor->trace(T*) mark the object and call traceWrapper of the object if T is ScriptWrappable - making visitor->trace(T*) just call traceWrapper of the object if T is not ScriptWrappable What do you think?
https://codereview.chromium.org/1876383003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h (right): https://codereview.chromium.org/1876383003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h:56: class ScriptWrappableVisitor { Done https://codereview.chromium.org/1876383003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Node.idl (right): https://codereview.chromium.org/1876383003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.idl:25: TraceWrappableReferences=(ownerDocument,parentNode,parentElement,nextSibling,previousSibling,firstChild,maybeRareData) Thank you for your wonderful comments! To be honest, most of the "complexities" of visiting were relics of past approaches and were plain needless in the current approach. So thank you for catching that :) I refactored the code, looking much better now. Ptal. Regarding code generation, let me sum up the pros and cons: pros: * all 53 cases of SetWrapperReferenceFrom could be just sed-ded to TraceWrappableReferences * all 34 cases of SetWrapperReferenceTo could be sed-ded after refactoring in https://codereview.chromium.org/1876843002 * for the rest of more than 1000 idl files, I don't have exact numbers, but the feeling is, that most of the wrappable classes already have const getters * declarative nature, "just tell us the dependencies and we will trace them for you" cons: * inconsistency, some stuff is generated, some hand written * inconsistency with oilpan, as all oilpan tracing methods are hand written * you still have to update header file + idl file (instead of header file and cpp file) - not a big win * we need the refactoring in https://codereview.chromium.org/1876843002 to generate traceWrappers for dom/Node and dom/Element, the most complex ones. As there is not a huge win in generation, and you are against it, I will remove it, if you haven't changed your mind :) p.s. I will cleanup V8 side naming (e.g. from TraceWrappablesFrom to TraceWrappersFrom) once we land this cl.
Thanks for the update -- the latest CL looks much tidier and nicer! On 2016/04/13 08:46:09, Marcel Hlopko wrote: > https://codereview.chromium.org/1876383003/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h > (right): > > https://codereview.chromium.org/1876383003/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h:56: class > ScriptWrappableVisitor { > Done > > https://codereview.chromium.org/1876383003/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/dom/Node.idl (right): > > https://codereview.chromium.org/1876383003/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/dom/Node.idl:25: > TraceWrappableReferences=(ownerDocument,parentNode,parentElement,nextSibling,previousSibling,firstChild,maybeRareData) > Thank you for your wonderful comments! > > To be honest, most of the "complexities" of visiting were relics of past > approaches and were plain needless in the current approach. So thank you for > catching that :) > > I refactored the code, looking much better now. Ptal. > > Regarding code generation, let me sum up the pros and cons: > > pros: > * all 53 cases of SetWrapperReferenceFrom could be just sed-ded to > TraceWrappableReferences > * all 34 cases of SetWrapperReferenceTo could be sed-ded after refactoring in > https://codereview.chromium.org/1876843002 > * for the rest of more than 1000 idl files, I don't have exact numbers, but the > feeling is, that most of the wrappable classes already have const getters > * declarative nature, "just tell us the dependencies and we will trace them for > you" > cons: > * inconsistency, some stuff is generated, some hand written > * inconsistency with oilpan, as all oilpan tracing methods are hand written > * you still have to update header file + idl file (instead of header file and > cpp file) - not a big win > * we need the refactoring in https://codereview.chromium.org/1876843002 to > generate traceWrappers for dom/Node and dom/Element, the most complex ones. Yeah, my main concern for the generated tracing method is that we'll anyway need to hand-write many things. Other than [SetWrapperReferenceFrom/To], another important goal is to remove ScopedPersistents and ScriptValues by tracing the handles (then we can get rid of leaks caused by those strong persistent handles from Blink to V8). As far as I grep in the code base, there are many ScopedPersistents and ScriptValues retained by DOM objects that are not ScriptWrappable. This means that we need to hand-write a tracing method for them. If we anyway need to hand-write many tracing methods, I guess it would be more straightforward to adopt the same programming pattern as Oilpan (for consistency). What do you think? > As there is not a huge win in generation, and you are against it, I will remove > it, if you haven't changed your mind :) > > p.s. > I will cleanup V8 side naming (e.g. from TraceWrappablesFrom to > TraceWrappersFrom) once we land this cl.
On 2016/04/13 at 09:05:22, haraken wrote: > Thanks for the update -- the latest CL looks much tidier and nicer! > > On 2016/04/13 08:46:09, Marcel Hlopko wrote: > > https://codereview.chromium.org/1876383003/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h > > (right): > > > > https://codereview.chromium.org/1876383003/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h:56: class > > ScriptWrappableVisitor { > > Done > > > > https://codereview.chromium.org/1876383003/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/dom/Node.idl (right): > > > > https://codereview.chromium.org/1876383003/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/dom/Node.idl:25: > > TraceWrappableReferences=(ownerDocument,parentNode,parentElement,nextSibling,previousSibling,firstChild,maybeRareData) > > Thank you for your wonderful comments! > > > > To be honest, most of the "complexities" of visiting were relics of past > > approaches and were plain needless in the current approach. So thank you for > > catching that :) > > > > I refactored the code, looking much better now. Ptal. > > > > Regarding code generation, let me sum up the pros and cons: > > > > pros: > > * all 53 cases of SetWrapperReferenceFrom could be just sed-ded to > > TraceWrappableReferences > > * all 34 cases of SetWrapperReferenceTo could be sed-ded after refactoring in > > https://codereview.chromium.org/1876843002 > > * for the rest of more than 1000 idl files, I don't have exact numbers, but the > > feeling is, that most of the wrappable classes already have const getters > > * declarative nature, "just tell us the dependencies and we will trace them for > > you" > > cons: > > * inconsistency, some stuff is generated, some hand written > > * inconsistency with oilpan, as all oilpan tracing methods are hand written > > * you still have to update header file + idl file (instead of header file and > > cpp file) - not a big win > > * we need the refactoring in https://codereview.chromium.org/1876843002 to > > generate traceWrappers for dom/Node and dom/Element, the most complex ones. > > Yeah, my main concern for the generated tracing method is that we'll anyway need to hand-write many things. > > Other than [SetWrapperReferenceFrom/To], another important goal is to remove ScopedPersistents and ScriptValues by tracing the handles (then we can get rid of leaks caused by those strong persistent handles from Blink to V8). As far as I grep in the code base, there are many ScopedPersistents and ScriptValues retained by DOM objects that are not ScriptWrappable. This means that we need to hand-write a tracing method for them. > > If we anyway need to hand-write many tracing methods, I guess it would be more straightforward to adopt the same programming pattern as Oilpan (for consistency). What do you think? > My feeling is that if we can generate 1000+ methods that's 1000+ less places that can be accidentially buggy.
On 2016/04/13 at 09:51:12, jochen - slow wrote: > On 2016/04/13 at 09:05:22, haraken wrote: > > Thanks for the update -- the latest CL looks much tidier and nicer! > > > > On 2016/04/13 08:46:09, Marcel Hlopko wrote: > > > https://codereview.chromium.org/1876383003/diff/20001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h > > > (right): > > > > > > https://codereview.chromium.org/1876383003/diff/20001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h:56: class > > > ScriptWrappableVisitor { > > > Done > > > > > > https://codereview.chromium.org/1876383003/diff/20001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/core/dom/Node.idl (right): > > > > > > https://codereview.chromium.org/1876383003/diff/20001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/core/dom/Node.idl:25: > > > TraceWrappableReferences=(ownerDocument,parentNode,parentElement,nextSibling,previousSibling,firstChild,maybeRareData) > > > Thank you for your wonderful comments! > > > > > > To be honest, most of the "complexities" of visiting were relics of past > > > approaches and were plain needless in the current approach. So thank you for > > > catching that :) > > > > > > I refactored the code, looking much better now. Ptal. > > > > > > Regarding code generation, let me sum up the pros and cons: > > > > > > pros: > > > * all 53 cases of SetWrapperReferenceFrom could be just sed-ded to > > > TraceWrappableReferences > > > * all 34 cases of SetWrapperReferenceTo could be sed-ded after refactoring in > > > https://codereview.chromium.org/1876843002 > > > * for the rest of more than 1000 idl files, I don't have exact numbers, but the > > > feeling is, that most of the wrappable classes already have const getters > > > * declarative nature, "just tell us the dependencies and we will trace them for > > > you" > > > cons: > > > * inconsistency, some stuff is generated, some hand written > > > * inconsistency with oilpan, as all oilpan tracing methods are hand written > > > * you still have to update header file + idl file (instead of header file and > > > cpp file) - not a big win > > > * we need the refactoring in https://codereview.chromium.org/1876843002 to > > > generate traceWrappers for dom/Node and dom/Element, the most complex ones. > > > > Yeah, my main concern for the generated tracing method is that we'll anyway need to hand-write many things. > > > > Other than [SetWrapperReferenceFrom/To], another important goal is to remove ScopedPersistents and ScriptValues by tracing the handles (then we can get rid of leaks caused by those strong persistent handles from Blink to V8). As far as I grep in the code base, there are many ScopedPersistents and ScriptValues retained by DOM objects that are not ScriptWrappable. This means that we need to hand-write a tracing method for them. > > > > If we anyway need to hand-write many tracing methods, I guess it would be more straightforward to adopt the same programming pattern as Oilpan (for consistency). What do you think? > > > > My feeling is that if we can generate 1000+ methods that's 1000+ less places that can be accidentially buggy. (or even just hundreds..)
https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp (right): https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp:54: void ScriptWrappableVisitor::trace(const NodeRareData* data) const Do we need to add a trace(const T*) method every time we trace a DOM object that is not ScriptWrappable?
https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp (right): https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp:54: void ScriptWrappableVisitor::trace(const NodeRareData* data) const On 2016/04/13 at 11:09:05, haraken wrote: > Do we need to add a trace(const T*) method every time we trace a DOM object that is not ScriptWrappable? Yes. Do you have a better approach in mind?
On 2016/04/13 11:11:57, Marcel Hlopko wrote: > https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp > (right): > > https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp:54: void > ScriptWrappableVisitor::trace(const NodeRareData* data) const > On 2016/04/13 at 11:09:05, haraken wrote: > > Do we need to add a trace(const T*) method every time we trace a DOM object > that is not ScriptWrappable? > > Yes. Do you have a better approach in mind? Not really. OK, so my suggestion is: - I'm not yet fully convinced that the auto-generate approach is better. At some point we need to trace many DOM objects that don't have IDL files. Then the tracing logic will be scattered in the code base (IDL files + ScriptWrapableVisitor::trace + hand-written trace method on the DOM object), which will make it harder to understand what objects are traced. (If we can generate almost all tracing code from IDL files, I'm a fan of the idea though.) - That said, I don't object to proceeding with the auto-generate approach in v0 if you think that's better.
I think nobody is fully convinced about superiority of any of generate/not generate approaches. So let's keep the generation there and decide later, as the corner cases unwrap themselves. I'll try *not* to introduce const getters all over the place (so we don't have to remove them when generation turns out useless). We can always introduce const getters to cleanup the code later. If you all agree, we move on to the actual cl. Especially the vector holding the marked headers (to unmark them later) is not very elegant solution, but the best I could come up so far. Comments welcomed.
On 2016/04/13 11:37:38, Marcel Hlopko wrote: > I think nobody is fully convinced about superiority of any of generate/not > generate approaches. So let's keep the generation there and decide later, as the > corner cases unwrap themselves. I'll try *not* to introduce const getters all > over the place (so we don't have to remove them when generation turns out > useless). We can always introduce const getters to cleanup the code later. > > If you all agree, we move on to the actual cl. Especially the vector holding the > marked headers (to unmark them later) is not very elegant solution, but the best > I could come up so far. Comments welcomed. Sounds reasonable to me :)
:) So who wants to lgtm this thing? :)
My main comments are the following two points: - This CL will keep alive wrappers in the main world, but how are wrappers in isolated worlds kept alive? - It's not really nice to keep a vector of HeapObjectHeader* in ScriptWrappableTracer. If an Oilpan's GC is triggered, the HeapObjectHeader* can get stale (even though it won't happen at this point because you haven't yet implemented incremental marking). This is why I don't think it's a good idea to use Oilpan's header for the bit. Conceptually the traceWrapper should work on ScriptWrappables, so you should use a bit on the ScriptWrappable object (e.g., the least significant bit). https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/IDLExtendedAttributes.txt (right): https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/IDLExtendedAttributes.txt:91: TraceWrapperReferences=* Please add a binding test for this IDL attribute (Source/bindings/tests/). https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ActiveScriptWrappable.cpp (right): https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ActiveScriptWrappable.cpp:42: void ActiveScriptWrappable::traceActiveWrappable(ScriptWrappableVisitor* visitor) traceActiveWrappable => traceWrappers ? (I want to unify the name of the tracing methods for consistency.) https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptWrappable.cpp (right): https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptWrappable.cpp:38: void ScriptWrappable::markWrapperAlive(const v8::Persistent<v8::Object>& handle, v8::Isolate* isolate) I think this static method should be moved to ScriptWrappableHeapTracer. ScriptWrappableHeapTracer should be responsible for proxying things to V8 APIs. https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptWrappable.cpp:40: handle.RegisterExternalReference(isolate); This will keep alive the wrapper of the main world, but what about wrappers in other isolated worlds? https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptWrappable.h (right): https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptWrappable.h:154: void markWrapperAlive(v8::Isolate* isolate) const markWrapperAlive => markWrappers? https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.cpp (right): https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.cpp:40: void ScriptWrappableHeapTracer::TraceWrappableFrom(v8::Isolate* isolate, const std::vector<std::pair<void*, void*>>& hiddenFieldsOfPotentialWrappers) Would you help me understand what this method does? https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.cpp:49: void ScriptWrappableHeapTracer::TraceWrappableFrom(v8::Isolate* isolate, std::pair<void*, void*> hiddenFields) What is the hidden fields? https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.cpp:53: if (reinterpret_cast<intptr_t>(hiddenFields.first) % 2 == 1) What is this checking? https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.h (right): https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.h:32: std::vector<HeapObjectHeader*> m_headersToUnmark; How is it guaranteed that Oilpan's GC does not happen while V8 is doing tracing? (Maybe it's guaranteed at this point because you haven't implement incremental marking?) If that happens, the HeapObjectHeader* can be a stale pointer. Actually this is (one of) the reasons I think it's not a good idea to use Oilpan's header for the bit. Conceptually the bit should be set on ScriptWrappabl. You can use the least significant bit of the ScriptWrappable object. https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp (right): https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp:25: if (scriptWrappable->containsWrapper()) It would be better to move this check into ScriptWrappable::markWrapperAlive. https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h (right): https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h:20: #define DECLARE_VIRTUAL_TRACE_WRAPPERS() \ Add a comment how to use these macros, or add a link to the IDLExtendedAttributes.md. https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h:26: #define OVERRIDE_TRACE_WRAPPERS() \ Can we unify OVERRIDE_TRACE_WRAPPERS with DECLARE_VIRTUAL_TRACE_WRAPPERS? https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h:33: * Visitor able to trace through the script wrappable references. It is used Visitor is able to https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h:36: class ScriptWrappableVisitor { Add STACK_ALLOCATED. https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h:46: void mark(const void* garbageCollected) const; I'd suggest consistently using "traceWrappers" and "markWrappers" for all these methods. Then it would be less confusing with Oilpan's ones. (Note that developers soemtimes need to mix Oilpan's tracing methods and Wrappable's tracing methods in the same file.) https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8GCController.h (right): https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8GCController.h:43: class ScriptWrappableVisitor; Unnecessary change. https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Node.cpp (right): https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.cpp:342: NodeRareData* Node::maybeRareData() const maybeRareData => rareDataForTraceWrappers (to clarify this method is intended only for traceWrappers) https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/HeapPage.h (right): https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/HeapPage.h:210: void unmarkDOM(); isDOMMarked => isWrappableMarked markDOM => markWrappable unmarkDOM => unmarkWrappable https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/HeapPage.h:844: return m_encoded & headerDOMMarkBitMask; Where is headerDOMMarkBitMask defined?
Ptaal :) (the cl is not currently buildable as the accompanying v8 cl has not yet landed) https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/IDLExtendedAttributes.txt (right): https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/IDLExtendedAttributes.txt:91: TraceWrapperReferences=* Done. https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ActiveScriptWrappable.cpp (right): https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ActiveScriptWrappable.cpp:42: void ActiveScriptWrappable::traceActiveWrappable(ScriptWrappableVisitor* visitor) Done. https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptWrappable.cpp (right): https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptWrappable.cpp:38: void ScriptWrappable::markWrapperAlive(const v8::Persistent<v8::Object>& handle, v8::Isolate* isolate) Agreed. Done. https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptWrappable.cpp:40: handle.RegisterExternalReference(isolate); On 2016/04/13 at 12:26:22, haraken wrote: > This will keep alive the wrapper of the main world, but what about wrappers in other isolated worlds? For the first version, let's mark wrappers from all worlds when wrapper from any world is reachable. I changed the code accordingly - ptal. As for the future plans, we will have to pass context embedder data from V8, and visitor would know for which world it traces. For the incremental approach, we will have either 2 marking queues, one for the main world, one for all other (having one queue per world seems "too much"), or one marking queue with pairs <scriptWrappable,world>. But that is distant future, and we will discuss it more for sure. https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptWrappable.h (right): https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptWrappable.h:154: void markWrapperAlive(v8::Isolate* isolate) const Done. https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.cpp (right): https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.cpp:40: void ScriptWrappableHeapTracer::TraceWrappableFrom(v8::Isolate* isolate, const std::vector<std::pair<void*, void*>>& hiddenFieldsOfPotentialWrappers) On 2016/04/13 at 12:26:22, haraken wrote: > Would you help me understand what this method does? When marking, V8 detects all (possible) wrappers and collects their first two internal fields. That is enough for blink side to reconstruct script wrappable, and simple for V8. Had it been a vector of Objects, V8 would had to update references when it moves objects. https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.cpp:49: void ScriptWrappableHeapTracer::TraceWrappableFrom(v8::Isolate* isolate, std::pair<void*, void*> hiddenFields) On 2016/04/13 at 12:26:22, haraken wrote: > What is the hidden fields? Renamed to internal fields to be consistent with the v8 api - fields set by the embedder, not accessible from javascript, appearring as tagged integers to the V8 gc. It's all we need to reconstruct script wrappables. https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.cpp:53: if (reinterpret_cast<intptr_t>(hiddenFields.first) % 2 == 1) On 2016/04/13 at 12:26:22, haraken wrote: > What is this checking? That the pointer we got is a valid oilpan pointer, and not tagged pointer to the V8 object. Is ASSERT_NOT_REACHED correctly used here? https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.h (right): https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.h:32: std::vector<HeapObjectHeader*> m_headersToUnmark; On 2016/04/13 at 12:26:22, haraken wrote: > How is it guaranteed that Oilpan's GC does not happen while V8 is doing tracing? (Maybe it's guaranteed at this point because you haven't implement incremental marking?) > > If that happens, the HeapObjectHeader* can be a stale pointer. > > Actually this is (one of) the reasons I think it's not a good idea to use Oilpan's header for the bit. Conceptually the bit should be set on ScriptWrappabl. You can use the least significant bit of the ScriptWrappable object. Currently, there are no guarantees that oilpan gc does not happen, but I'm not sure it is a problem. The fact that we marked a header means the object is reachable from some wrapper. And for oilpan gc, all wrappers will keep their script wrappables alive. Oilpan's reachability graph is supergraph of wrapper reachability graph, therefore marked headers should not be deleted by the oilpan gc. As long as oilpan doesn't move objects/headers, we should be fine. Or did I miss something? Using a bit in the ScriptWrappable is a good idea, however not all objects we trace through are script wrappable. I am not aware of a cycle which would make us trace forever, but still, we might visit non-wrappables multiple times. And to be honest I have no idea what impact this can have in the real world, therefore I lean to "safer" oilpan header marking. Regarding the future plans for the incremental tracing, wrappable-marked objects and objects on the marking queue should be considered as alive by oilpan. Wdyt? https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp (right): https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp:25: if (scriptWrappable->containsWrapper()) Done. https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h (right): https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h:26: #define OVERRIDE_TRACE_WRAPPERS() \ oOn 2016/04/13 at 12:26:22, haraken wrote: > Can we unify OVERRIDE_TRACE_WRAPPERS with DECLARE_VIRTUAL_TRACE_WRAPPERS? Did you mean what I just uploaded, or you meant having only 1 macro for both? https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h:33: * Visitor able to trace through the script wrappable references. It is used Done. https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h:36: class ScriptWrappableVisitor { Done. https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h:46: void mark(const void* garbageCollected) const; Done. https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8GCController.h (right): https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8GCController.h:43: class ScriptWrappableVisitor; Done. https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Node.cpp (right): https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.cpp:342: NodeRareData* Node::maybeRareData() const On 2016/04/13 at 12:26:22, haraken wrote: > maybeRareData => rareDataForTraceWrappers (to clarify this method is intended only for traceWrappers) Removed completely, I'll write traceWrappers for Node by hand (in order not to introduce too many "useless" const getters). It was there to demonstrate how interaction with non-scriptWrappable will look like, and I think it is clear now :) https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/HeapPage.h (right): https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/HeapPage.h:210: void unmarkDOM(); Done. https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/HeapPage.h:844: return m_encoded & headerDOMMarkBitMask; On 2016/04/13 at 12:26:22, haraken wrote: > Where is headerDOMMarkBitMask defined? HeapPage.h:145 const size_t headerDOMMarkBitMask = 1u << 17; Renaming to headerWrappableMarkBitMask for consistency.
https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptWrappable.cpp (right): https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptWrappable.cpp:40: handle.RegisterExternalReference(isolate); On 2016/04/14 16:39:08, Marcel Hlopko wrote: > On 2016/04/13 at 12:26:22, haraken wrote: > > This will keep alive the wrapper of the main world, but what about wrappers in > other isolated worlds? > > For the first version, let's mark wrappers from all worlds when wrapper from any > world is reachable. I changed the code accordingly - ptal. > As for the future plans, we will have to pass context embedder data from V8, and > visitor would know for which world it traces. For the incremental > approach, we will have either 2 marking queues, one for the main world, one for > all other (having one queue per world seems "too much"), or one > marking queue with pairs <scriptWrappable,world>. But that is distant future, > and we will discuss it more for sure. This sounds like a good plan. For the first version, let's just mark wrappers in all worlds, which will keep the existing behavior. https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.h (right): https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.h:32: std::vector<HeapObjectHeader*> m_headersToUnmark; On 2016/04/14 16:39:08, Marcel Hlopko wrote: > On 2016/04/13 at 12:26:22, haraken wrote: > > How is it guaranteed that Oilpan's GC does not happen while V8 is doing > tracing? (Maybe it's guaranteed at this point because you haven't implement > incremental marking?) > > > > If that happens, the HeapObjectHeader* can be a stale pointer. > > > > Actually this is (one of) the reasons I think it's not a good idea to use > Oilpan's header for the bit. Conceptually the bit should be set on > ScriptWrappabl. You can use the least significant bit of the ScriptWrappable > object. > > Currently, there are no guarantees that oilpan gc does not happen, but I'm not > sure it is a problem. The fact that we marked a header means the object is > reachable from some wrapper. And for oilpan gc, all wrappers will keep their > script wrappables alive. Oilpan's reachability graph is supergraph of wrapper > reachability graph, therefore marked headers should not be deleted by the oilpan > gc. As long as oilpan doesn't move objects/headers, we should be fine. Or did I > miss something? Ah, good point. You're right. > > Using a bit in the ScriptWrappable is a good idea, however not all objects we > trace through are script wrappable. I am not aware of a cycle which would make > us trace forever, but still, we might visit non-wrappables multiple times. And > to be honest I have no idea what impact this can have in the real world, > therefore I lean to "safer" oilpan header marking. > > Regarding the future plans for the incremental tracing, wrappable-marked objects > and objects on the marking queue should be considered as alive by oilpan. Wdyt? Currently all ScriptWrappables are guaranteed to be on Oilpan's heap, but not all DOM objects that are going to be traced by traceWrappers are on Oilpan's heap. So you cannot assume that all objects that have traceWrappers() have Oilpan's header. However, I think this is fixable -- i.e., it would be possible to move all those objects to Oilpan's heap. BTW, how are you planning to implement an incremental marking? Assume that we have the following object graph in the DOM side. Also assume that all the objects are on Oilpan's heap: ScriptWrappable1 => NonScriptWrappable1 => NonScriptWrappable2 => ScriptWrappable2 If a mutator (i.e., Blink) updates the object graph as follows during the incremental marking, how can we detect it? ScriptWrappable1 => NonScriptWrappable1 => NonScriptWrappable3 => ... Are you planning to implement a write/delete barrier to all Member<> updates in Oilpan's heap? In terms of performance, it would be unacceptable to insert write barriers to all Member<> updates but acceptable to insert write barriers to selected Member<>s (which get involved in traceWrappers()). However, it will require a substantial amount of work in Oilpan's infrastructure.
https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.h (right): https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.h:32: std::vector<HeapObjectHeader*> m_headersToUnmark; On 2016/04/15 at 01:28:16, haraken wrote: > On 2016/04/14 16:39:08, Marcel Hlopko wrote: > > On 2016/04/13 at 12:26:22, haraken wrote: > > > How is it guaranteed that Oilpan's GC does not happen while V8 is doing > > tracing? (Maybe it's guaranteed at this point because you haven't implement > > incremental marking?) > > > > > > If that happens, the HeapObjectHeader* can be a stale pointer. > > > > > > Actually this is (one of) the reasons I think it's not a good idea to use > > Oilpan's header for the bit. Conceptually the bit should be set on > > ScriptWrappabl. You can use the least significant bit of the ScriptWrappable > > object. > > > > Currently, there are no guarantees that oilpan gc does not happen, but I'm not > > sure it is a problem. The fact that we marked a header means the object is > > reachable from some wrapper. And for oilpan gc, all wrappers will keep their > > script wrappables alive. Oilpan's reachability graph is supergraph of wrapper > > reachability graph, therefore marked headers should not be deleted by the oilpan > > gc. As long as oilpan doesn't move objects/headers, we should be fine. Or did I > > miss something? > > Ah, good point. You're right. > > > > > Using a bit in the ScriptWrappable is a good idea, however not all objects we > > trace through are script wrappable. I am not aware of a cycle which would make > > us trace forever, but still, we might visit non-wrappables multiple times. And > > to be honest I have no idea what impact this can have in the real world, > > therefore I lean to "safer" oilpan header marking. > > > > Regarding the future plans for the incremental tracing, wrappable-marked objects > > and objects on the marking queue should be considered as alive by oilpan. Wdyt? > > Currently all ScriptWrappables are guaranteed to be on Oilpan's heap, but not all DOM objects that are going to be traced by traceWrappers are on Oilpan's heap. So you cannot assume that all objects that have traceWrappers() have Oilpan's header. However, I think this is fixable -- i.e., it would be possible to move all those objects to Oilpan's heap. > > BTW, how are you planning to implement an incremental marking? Assume that we have the following object graph in the DOM side. Also assume that all the objects are on Oilpan's heap: > > ScriptWrappable1 => NonScriptWrappable1 => NonScriptWrappable2 => ScriptWrappable2 > > If a mutator (i.e., Blink) updates the object graph as follows during the incremental marking, how can we detect it? > > ScriptWrappable1 => NonScriptWrappable1 => NonScriptWrappable3 => ... > > Are you planning to implement a write/delete barrier to all Member<> updates in Oilpan's heap? > > In terms of performance, it would be unacceptable to insert write barriers to all Member<> updates but acceptable to insert write barriers to selected Member<>s (which get involved in traceWrappers()). However, it will require a substantial amount of work in Oilpan's infrastructure. Sad, I was hoping the DOMArrayBuffer and DOMArrayBufferView were the last relevant not on the oilpan heap, they were only last script wrappables, right? But then moving the object to oilpan (especially if they are going to be moved anyway) is the cleanest solution. For the incremental marking, our plan is to manually insert write barriers into the relevant places, which will be time consuming and bug prone. We definitely don't want to add write barriers to all oilpan writes (by e.g. hooking into Member somehow). I will polish the design doc and send you a link so we can have more visible discussion there.
On 2016/04/15 07:27:15, Marcel Hlopko wrote: > https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.h > (right): > > https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.h:32: > std::vector<HeapObjectHeader*> m_headersToUnmark; > On 2016/04/15 at 01:28:16, haraken wrote: > > On 2016/04/14 16:39:08, Marcel Hlopko wrote: > > > On 2016/04/13 at 12:26:22, haraken wrote: > > > > How is it guaranteed that Oilpan's GC does not happen while V8 is doing > > > tracing? (Maybe it's guaranteed at this point because you haven't implement > > > incremental marking?) > > > > > > > > If that happens, the HeapObjectHeader* can be a stale pointer. > > > > > > > > Actually this is (one of) the reasons I think it's not a good idea to use > > > Oilpan's header for the bit. Conceptually the bit should be set on > > > ScriptWrappabl. You can use the least significant bit of the ScriptWrappable > > > object. > > > > > > Currently, there are no guarantees that oilpan gc does not happen, but I'm > not > > > sure it is a problem. The fact that we marked a header means the object is > > > reachable from some wrapper. And for oilpan gc, all wrappers will keep their > > > script wrappables alive. Oilpan's reachability graph is supergraph of > wrapper > > > reachability graph, therefore marked headers should not be deleted by the > oilpan > > > gc. As long as oilpan doesn't move objects/headers, we should be fine. Or > did I > > > miss something? > > > > Ah, good point. You're right. > > > > > > > > Using a bit in the ScriptWrappable is a good idea, however not all objects > we > > > trace through are script wrappable. I am not aware of a cycle which would > make > > > us trace forever, but still, we might visit non-wrappables multiple times. > And > > > to be honest I have no idea what impact this can have in the real world, > > > therefore I lean to "safer" oilpan header marking. > > > > > > Regarding the future plans for the incremental tracing, wrappable-marked > objects > > > and objects on the marking queue should be considered as alive by oilpan. > Wdyt? > > > > Currently all ScriptWrappables are guaranteed to be on Oilpan's heap, but not > all DOM objects that are going to be traced by traceWrappers are on Oilpan's > heap. So you cannot assume that all objects that have traceWrappers() have > Oilpan's header. However, I think this is fixable -- i.e., it would be possible > to move all those objects to Oilpan's heap. > > > > BTW, how are you planning to implement an incremental marking? Assume that we > have the following object graph in the DOM side. Also assume that all the > objects are on Oilpan's heap: > > > > ScriptWrappable1 => NonScriptWrappable1 => NonScriptWrappable2 => > ScriptWrappable2 > > > > If a mutator (i.e., Blink) updates the object graph as follows during the > incremental marking, how can we detect it? > > > > ScriptWrappable1 => NonScriptWrappable1 => NonScriptWrappable3 => ... > > > > Are you planning to implement a write/delete barrier to all Member<> updates > in Oilpan's heap? > > > > In terms of performance, it would be unacceptable to insert write barriers to > all Member<> updates but acceptable to insert write barriers to selected > Member<>s (which get involved in traceWrappers()). However, it will require a > substantial amount of work in Oilpan's infrastructure. > > Sad, I was hoping the DOMArrayBuffer and DOMArrayBufferView were the last > relevant not on the oilpan heap, they were only last script wrappables, right? > But then moving the object to oilpan (especially if they are going to be moved > anyway) is the cleanest solution. > > For the incremental marking, our plan is to manually insert write barriers into > the relevant places, which will be time consuming and bug prone. We definitely > don't want to add write barriers to all oilpan writes (by e.g. hooking into > Member somehow). I will polish the design doc and send you a link so we can have > more visible discussion there. Thanks. Although we've been discussing some issues in private emails (class ordering of GarbageCollected, ScriptWrappable etc), as far as I understand, this CL is ready for landing, right? Then I'll take a final look by Monday.
On 2016/04/15 at 11:39:44, haraken wrote: > On 2016/04/15 07:27:15, Marcel Hlopko wrote: > > https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.h > > (right): > > > > https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.h:32: > > std::vector<HeapObjectHeader*> m_headersToUnmark; > > On 2016/04/15 at 01:28:16, haraken wrote: > > > On 2016/04/14 16:39:08, Marcel Hlopko wrote: > > > > On 2016/04/13 at 12:26:22, haraken wrote: > > > > > How is it guaranteed that Oilpan's GC does not happen while V8 is doing > > > > tracing? (Maybe it's guaranteed at this point because you haven't implement > > > > incremental marking?) > > > > > > > > > > If that happens, the HeapObjectHeader* can be a stale pointer. > > > > > > > > > > Actually this is (one of) the reasons I think it's not a good idea to use > > > > Oilpan's header for the bit. Conceptually the bit should be set on > > > > ScriptWrappabl. You can use the least significant bit of the ScriptWrappable > > > > object. > > > > > > > > Currently, there are no guarantees that oilpan gc does not happen, but I'm > > not > > > > sure it is a problem. The fact that we marked a header means the object is > > > > reachable from some wrapper. And for oilpan gc, all wrappers will keep their > > > > script wrappables alive. Oilpan's reachability graph is supergraph of > > wrapper > > > > reachability graph, therefore marked headers should not be deleted by the > > oilpan > > > > gc. As long as oilpan doesn't move objects/headers, we should be fine. Or > > did I > > > > miss something? > > > > > > Ah, good point. You're right. > > > > > > > > > > > Using a bit in the ScriptWrappable is a good idea, however not all objects > > we > > > > trace through are script wrappable. I am not aware of a cycle which would > > make > > > > us trace forever, but still, we might visit non-wrappables multiple times. > > And > > > > to be honest I have no idea what impact this can have in the real world, > > > > therefore I lean to "safer" oilpan header marking. > > > > > > > > Regarding the future plans for the incremental tracing, wrappable-marked > > objects > > > > and objects on the marking queue should be considered as alive by oilpan. > > Wdyt? > > > > > > Currently all ScriptWrappables are guaranteed to be on Oilpan's heap, but not > > all DOM objects that are going to be traced by traceWrappers are on Oilpan's > > heap. So you cannot assume that all objects that have traceWrappers() have > > Oilpan's header. However, I think this is fixable -- i.e., it would be possible > > to move all those objects to Oilpan's heap. > > > > > > BTW, how are you planning to implement an incremental marking? Assume that we > > have the following object graph in the DOM side. Also assume that all the > > objects are on Oilpan's heap: > > > > > > ScriptWrappable1 => NonScriptWrappable1 => NonScriptWrappable2 => > > ScriptWrappable2 > > > > > > If a mutator (i.e., Blink) updates the object graph as follows during the > > incremental marking, how can we detect it? > > > > > > ScriptWrappable1 => NonScriptWrappable1 => NonScriptWrappable3 => ... > > > > > > Are you planning to implement a write/delete barrier to all Member<> updates > > in Oilpan's heap? > > > > > > In terms of performance, it would be unacceptable to insert write barriers to > > all Member<> updates but acceptable to insert write barriers to selected > > Member<>s (which get involved in traceWrappers()). However, it will require a > > substantial amount of work in Oilpan's infrastructure. > > > > Sad, I was hoping the DOMArrayBuffer and DOMArrayBufferView were the last > > relevant not on the oilpan heap, they were only last script wrappables, right? > > But then moving the object to oilpan (especially if they are going to be moved > > anyway) is the cleanest solution. > > > > For the incremental marking, our plan is to manually insert write barriers into > > the relevant places, which will be time consuming and bug prone. We definitely > > don't want to add write barriers to all oilpan writes (by e.g. hooking into > > Member somehow). I will polish the design doc and send you a link so we can have > > more visible discussion there. > > Thanks. > > Although we've been discussing some issues in private emails (class ordering of GarbageCollected, ScriptWrappable etc), as far as I understand, this CL is ready for landing, right? Then I'll take a final look by Monday. I would say so. I'm investigating the class ordering issue, but that will be part of future cl anyway. I would land this one as it is (if no comments arise). Thanks!
On 2016/04/15 11:50:06, Marcel Hlopko wrote: > On 2016/04/15 at 11:39:44, haraken wrote: > > On 2016/04/15 07:27:15, Marcel Hlopko wrote: > > > > https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.h > > > (right): > > > > > > > https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.h:32: > > > std::vector<HeapObjectHeader*> m_headersToUnmark; > > > On 2016/04/15 at 01:28:16, haraken wrote: > > > > On 2016/04/14 16:39:08, Marcel Hlopko wrote: > > > > > On 2016/04/13 at 12:26:22, haraken wrote: > > > > > > How is it guaranteed that Oilpan's GC does not happen while V8 is > doing > > > > > tracing? (Maybe it's guaranteed at this point because you haven't > implement > > > > > incremental marking?) > > > > > > > > > > > > If that happens, the HeapObjectHeader* can be a stale pointer. > > > > > > > > > > > > Actually this is (one of) the reasons I think it's not a good idea to > use > > > > > Oilpan's header for the bit. Conceptually the bit should be set on > > > > > ScriptWrappabl. You can use the least significant bit of the > ScriptWrappable > > > > > object. > > > > > > > > > > Currently, there are no guarantees that oilpan gc does not happen, but > I'm > > > not > > > > > sure it is a problem. The fact that we marked a header means the object > is > > > > > reachable from some wrapper. And for oilpan gc, all wrappers will keep > their > > > > > script wrappables alive. Oilpan's reachability graph is supergraph of > > > wrapper > > > > > reachability graph, therefore marked headers should not be deleted by > the > > > oilpan > > > > > gc. As long as oilpan doesn't move objects/headers, we should be fine. > Or > > > did I > > > > > miss something? > > > > > > > > Ah, good point. You're right. > > > > > > > > > > > > > > Using a bit in the ScriptWrappable is a good idea, however not all > objects > > > we > > > > > trace through are script wrappable. I am not aware of a cycle which > would > > > make > > > > > us trace forever, but still, we might visit non-wrappables multiple > times. > > > And > > > > > to be honest I have no idea what impact this can have in the real world, > > > > > therefore I lean to "safer" oilpan header marking. > > > > > > > > > > Regarding the future plans for the incremental tracing, wrappable-marked > > > objects > > > > > and objects on the marking queue should be considered as alive by > oilpan. > > > Wdyt? > > > > > > > > Currently all ScriptWrappables are guaranteed to be on Oilpan's heap, but > not > > > all DOM objects that are going to be traced by traceWrappers are on Oilpan's > > > heap. So you cannot assume that all objects that have traceWrappers() have > > > Oilpan's header. However, I think this is fixable -- i.e., it would be > possible > > > to move all those objects to Oilpan's heap. > > > > > > > > BTW, how are you planning to implement an incremental marking? Assume that > we > > > have the following object graph in the DOM side. Also assume that all the > > > objects are on Oilpan's heap: > > > > > > > > ScriptWrappable1 => NonScriptWrappable1 => NonScriptWrappable2 => > > > ScriptWrappable2 > > > > > > > > If a mutator (i.e., Blink) updates the object graph as follows during the > > > incremental marking, how can we detect it? > > > > > > > > ScriptWrappable1 => NonScriptWrappable1 => NonScriptWrappable3 => ... > > > > > > > > Are you planning to implement a write/delete barrier to all Member<> > updates > > > in Oilpan's heap? > > > > > > > > In terms of performance, it would be unacceptable to insert write barriers > to > > > all Member<> updates but acceptable to insert write barriers to selected > > > Member<>s (which get involved in traceWrappers()). However, it will require > a > > > substantial amount of work in Oilpan's infrastructure. > > > > > > Sad, I was hoping the DOMArrayBuffer and DOMArrayBufferView were the last > > > relevant not on the oilpan heap, they were only last script wrappables, > right? > > > But then moving the object to oilpan (especially if they are going to be > moved > > > anyway) is the cleanest solution. > > > > > > For the incremental marking, our plan is to manually insert write barriers > into > > > the relevant places, which will be time consuming and bug prone. We > definitely > > > don't want to add write barriers to all oilpan writes (by e.g. hooking into > > > Member somehow). I will polish the design doc and send you a link so we can > have > > > more visible discussion there. > > > > Thanks. > > > > Although we've been discussing some issues in private emails (class ordering > of GarbageCollected, ScriptWrappable etc), as far as I understand, this CL is > ready for landing, right? Then I'll take a final look by Monday. > > I would say so. I'm investigating the class ordering issue, but that will be > part of future cl anyway. I would land this one as it is (if no comments arise). > Thanks! Makes sense. Your plan is to move traceWrappers() to GarbageCollected in a follow-up CL, right?
On 2016/04/15 at 11:52:00, haraken wrote: > On 2016/04/15 11:50:06, Marcel Hlopko wrote: > > On 2016/04/15 at 11:39:44, haraken wrote: > > > On 2016/04/15 07:27:15, Marcel Hlopko wrote: > > > > > > https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... > > > > File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.h > > > > (right): > > > > > > > > > > https://codereview.chromium.org/1876383003/diff/40001/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.h:32: > > > > std::vector<HeapObjectHeader*> m_headersToUnmark; > > > > On 2016/04/15 at 01:28:16, haraken wrote: > > > > > On 2016/04/14 16:39:08, Marcel Hlopko wrote: > > > > > > On 2016/04/13 at 12:26:22, haraken wrote: > > > > > > > How is it guaranteed that Oilpan's GC does not happen while V8 is > > doing > > > > > > tracing? (Maybe it's guaranteed at this point because you haven't > > implement > > > > > > incremental marking?) > > > > > > > > > > > > > > If that happens, the HeapObjectHeader* can be a stale pointer. > > > > > > > > > > > > > > Actually this is (one of) the reasons I think it's not a good idea to > > use > > > > > > Oilpan's header for the bit. Conceptually the bit should be set on > > > > > > ScriptWrappabl. You can use the least significant bit of the > > ScriptWrappable > > > > > > object. > > > > > > > > > > > > Currently, there are no guarantees that oilpan gc does not happen, but > > I'm > > > > not > > > > > > sure it is a problem. The fact that we marked a header means the object > > is > > > > > > reachable from some wrapper. And for oilpan gc, all wrappers will keep > > their > > > > > > script wrappables alive. Oilpan's reachability graph is supergraph of > > > > wrapper > > > > > > reachability graph, therefore marked headers should not be deleted by > > the > > > > oilpan > > > > > > gc. As long as oilpan doesn't move objects/headers, we should be fine. > > Or > > > > did I > > > > > > miss something? > > > > > > > > > > Ah, good point. You're right. > > > > > > > > > > > > > > > > > Using a bit in the ScriptWrappable is a good idea, however not all > > objects > > > > we > > > > > > trace through are script wrappable. I am not aware of a cycle which > > would > > > > make > > > > > > us trace forever, but still, we might visit non-wrappables multiple > > times. > > > > And > > > > > > to be honest I have no idea what impact this can have in the real world, > > > > > > therefore I lean to "safer" oilpan header marking. > > > > > > > > > > > > Regarding the future plans for the incremental tracing, wrappable-marked > > > > objects > > > > > > and objects on the marking queue should be considered as alive by > > oilpan. > > > > Wdyt? > > > > > > > > > > Currently all ScriptWrappables are guaranteed to be on Oilpan's heap, but > > not > > > > all DOM objects that are going to be traced by traceWrappers are on Oilpan's > > > > heap. So you cannot assume that all objects that have traceWrappers() have > > > > Oilpan's header. However, I think this is fixable -- i.e., it would be > > possible > > > > to move all those objects to Oilpan's heap. > > > > > > > > > > BTW, how are you planning to implement an incremental marking? Assume that > > we > > > > have the following object graph in the DOM side. Also assume that all the > > > > objects are on Oilpan's heap: > > > > > > > > > > ScriptWrappable1 => NonScriptWrappable1 => NonScriptWrappable2 => > > > > ScriptWrappable2 > > > > > > > > > > If a mutator (i.e., Blink) updates the object graph as follows during the > > > > incremental marking, how can we detect it? > > > > > > > > > > ScriptWrappable1 => NonScriptWrappable1 => NonScriptWrappable3 => ... > > > > > > > > > > Are you planning to implement a write/delete barrier to all Member<> > > updates > > > > in Oilpan's heap? > > > > > > > > > > In terms of performance, it would be unacceptable to insert write barriers > > to > > > > all Member<> updates but acceptable to insert write barriers to selected > > > > Member<>s (which get involved in traceWrappers()). However, it will require > > a > > > > substantial amount of work in Oilpan's infrastructure. > > > > > > > > Sad, I was hoping the DOMArrayBuffer and DOMArrayBufferView were the last > > > > relevant not on the oilpan heap, they were only last script wrappables, > > right? > > > > But then moving the object to oilpan (especially if they are going to be > > moved > > > > anyway) is the cleanest solution. > > > > > > > > For the incremental marking, our plan is to manually insert write barriers > > into > > > > the relevant places, which will be time consuming and bug prone. We > > definitely > > > > don't want to add write barriers to all oilpan writes (by e.g. hooking into > > > > Member somehow). I will polish the design doc and send you a link so we can > > have > > > > more visible discussion there. > > > > > > Thanks. > > > > > > Although we've been discussing some issues in private emails (class ordering > > of GarbageCollected, ScriptWrappable etc), as far as I understand, this CL is > > ready for landing, right? Then I'll take a final look by Monday. > > > > I would say so. I'm investigating the class ordering issue, but that will be > > part of future cl anyway. I would land this one as it is (if no comments arise). > > Thanks! > > Makes sense. Your plan is to move traceWrappers() to GarbageCollected in a follow-up CL, right? Ah, misunderstanding. I'm investigating reordering base classes, while keeping traceWrappers method on ScriptWrappable. I'm not sure about traceWrappers on GarbageCollected. Let me elaborate. Say I moved the traceWrappers to GarbageCollected. I will have 2 overloaded methods in the ScriptWrappableVisitor: one for GarbageCollected, one for ScriptWrappable (where I will collect actual wrappers). Then each class inheriting from both will have to cast itself as ScriptWrappable for visitor. And it will still not solve our header problem. But we wouldn't have to add a traceWrappers method for every non script wrappable object. If we made ScriptWrappable inherit from GarbageCollected, the situation would be different, but that craves for a new cl too. Or am I just too obtuse :) on Friday afternoon and I don't see the obvious elegant solution?
Added few fixes
Mostly looks good. Here is a final round of comments.
> Ah, misunderstanding. I'm investigating reordering base classes, while keeping
> traceWrappers method on ScriptWrappable. I'm not sure about traceWrappers on
> GarbageCollected. Let me elaborate. Say I moved the traceWrappers to
> GarbageCollected. I will have 2 overloaded methods in the
> ScriptWrappableVisitor: one for GarbageCollected, one for ScriptWrappable
(where
> I will collect actual wrappers). Then each class inheriting from both will
have
> to cast itself as ScriptWrappable for visitor. And it will still not solve our
> header problem. But we wouldn't have to add a traceWrappers method for every
non
> script wrappable object. If we made ScriptWrappable inherit from
> GarbageCollected, the situation would be different, but that craves for a new
cl
> too.
I think that making ScriptWrappable inherit from GarbageCollected would be the
cleanest solution. Then the header-positioning issue will be gone. You can just
define traceWrappers on GarbageCollected. Right?
That said, it will be an amount of work to make ScriptWrappable inherit from
GarbageCollected in practice. I'd propose, as a first step, you just reorder
base classes so that GarbageCollected and ScriptWrappable have the same begin
address (and add some runtime verification to verify the fact). This would be
enough for your purposes.
class X : public GarbageCollected<X>, public ScriptWrappable { };
Does it make sense?
https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/IDLExtendedAttributes.md (right):
https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/IDLExtendedAttributes.md:1284: The generator
generates a functions called `XXX::traceWrappers` which is called by
`ScriptWrappableHeapTracer` during V8 GC. The function adds references from the
XXX instance to this object's element1() and element2() children.
functions => function
https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ActiveScriptWrappable.h (right):
https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ActiveScriptWrappable.h:26: static
void traceActiveScriptWrappables(ScriptWrappableVisitor*);
traceActiveScriptWrappables => traceWrappers (for consistency)
https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/DOMDataStore.h (right):
https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMDataStore.h:159:
scriptWrappable->markWrapper(isolate);
Sorry, this would be a bug of the existing bindings... You'll never this branch
because all wrappers in the main world are stored in ScriptWrappables (i.e.,
doesn't use DOMDataStore at all).
If you ever hit this branch, I guess you'll run into an infinite tracing loop.
(I'll remove the m_isMainWorld and make sure that DOMDataStore is used only by
isolated worlds.)
https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/DOMWrapperMap.h (right):
https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/DOMWrapperMap.h:98:
m_map.RegisterExternallyReferencedObject(object);
I'm okay with this, but it would be nicer if all call sites of
RegisterExternallyReferencedObject are unified into
ScriptWrappableHeapTracer::markWrapper.
If you cannot do the unification, you can keep this, but then it would make more
sense to remove ScriptWrappableHeapTracer::markWrapper. Alternately,
ScriptWrappable::markWrapper can directly call
RegisterExternallyReferencedObject. (i.e., ScriptWrappable takes care of the
main world, and DOMWrapperMap takes care of isolated worlds.)
https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScriptWrappable.cpp (right):
https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappable.cpp:39: void
ScriptWrappable::markWrapper(v8::Isolate* isolate) const
This method looks redundant. The caller sites of this method should just call
ScriptWrappableHeapTracer::markWrapper.
(Below I'm suggesting renaming it to
ScriptWrappableHeapTracer::markWrappersInAllWorlds.)
https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.cpp
(right):
https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.cpp:32:
void ScriptWrappableHeapTracer::ClearTracingMarks(v8::Isolate* isolate)
TraceRoots => TracePrologue
ClearTracingMarks => TraceEpilogue
?
https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.cpp:41:
void ScriptWrappableHeapTracer::TraceWrappableFrom(v8::Isolate* isolate, const
std::vector<std::pair<void*, void*>>& internalFieldsOfPotentialWrappers)
std::vector is not allowed in Blink.
Can we use a visitor pattern (e.g., v8::PersistentHandleVisitor) and avoid
passing std::vector to Blink?
https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.cpp:55:
ASSERT_NOT_REACHED();
Then we can encapsulate this mysterious check in the V8 side.
https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.cpp:63: if
(classId != WrapperTypeInfo::NodeClassId
Actually I want to ask V8 to pass in the class ID to Blink so that Blink just
needs to check the class ID.
In other words, I want to design the ScriptWrappableHeapTracer in the same
manner as the v8::PersistentHandleVisitor.
https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.cpp:92:
void ScriptWrappableHeapTracer::markWrapper(const ScriptWrappable*
scriptWrappable, v8::Isolate* isolate)
markWrapper => markWrappersInAllWorlds ?
https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.cpp:97:
DOMWrapperWorld::visitAllWorldsInMainThread(visitor);
visitAllWorldsInMainThread doesn't much make sense because this tracing
mechanism should work for worker threads as well.
Also using a visitor pattern here might be a bit overkilling. We can probably
just introduce DOMWrapperWorld::markWrappersInAllWorlds(ScriptWrappable*), which
is responsible for marking all wrappers in both main world and isolated worlds?
https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.cpp:104:
dataStore.markWrapper(m_scriptWrappable, m_isolate);
And you can call this in
DOMWrapperWorld::markWrappersInAllWorlds(ScriptWrappable*).
Also add a TODO and mention that we should avoid tracing all wrappers in
isolated worlds. (Actually I'm a bit afraid of the performance overhead but
let's worry about the details later :-)
https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.h
(right):
https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.h:47:
std::vector<HeapObjectHeader*> m_headersToUnmark;
std::vector is not allowed in Blink. Use WTF::Vector instead.
https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.h:47:
std::vector<HeapObjectHeader*> m_headersToUnmark;
Add a comment why it's safe to hold raw pointers to HeapObjectHeader*.
https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp
(right):
https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp:43: void
ScriptWrappableVisitor::traceWrappers(const ScriptWrappable& wrappable) const
Is this method needed?
https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h
(right):
https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h:49:
DECLARE_VIRTUAL_TRACE_WRAPPERS() override;
Yeah, what I was suggesting is to remove OVERRIDE_TRACE_WRAPPERS() and just use
DECLARE_VIRTUAL_TRACE_WRAPPERS().
c.f., Oilpan uses DECLARE_VIRTUAL_TRACE() for both parent and child classes.
https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h:83: bool
isMarked(const void* garbageCollected) const;
mark => markWrapper
isMarked => isWrapperMarked
https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/tests/idls/core/TestTraceWrapperReferences.idl
(right):
https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/tests/idls/core/TestTraceWrapperReferences.idl:6:
TraceWrapperReferences=(firstChild,nextSibling,prevSibling,parent)
Nit: Can you add [TraceWrapperReferences] to some existing IDL interface? I want
to avoid adding a new interface since it will increase # of generated cpp files.
https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/NodeRareData.cpp (right):
https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/NodeRareData.cpp:68:
DEFINE_TRACE_WRAPPERS(NodeRareData)
Ditto.
https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/NodeRareData.h (right):
https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/NodeRareData.h:105:
DECLARE_NONVIRTUAL_TRACE_WRAPPERS();
This is unused in this CL. So remove this?
https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/HeapPage.h (right):
https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/HeapPage.h:211: bool isMarked() const;
headerWrappableMarkBitMask => headerWrapperMarkBitMask
isWrappableMarked => isWrapperMarked
markWrappable => markWrapper (for consistency)
unmarkWrappable => unmarkWrapper
wrt vectors: the std::vector is part of the V8 API, so it's technically allowed (and using WTF::Vector is not possible in the V8 API). I don't think it's a good idea to call for every single handle over the API boundary. V8 internally has a vector of to be visited objects. We'd now do one call per entry, collecting them on the blink side in a vector. This sounds like a huge, unnecessary overhead.
On 2016/04/18 09:35:46, jochen wrote: > wrt vectors: the std::vector is part of the V8 API, so it's technically allowed > (and using WTF::Vector is not possible in the V8 API). > > I don't think it's a good idea to call for every single handle over the API > boundary. V8 internally has a vector of to be visited objects. We'd now do one > call per entry, collecting them on the blink side in a vector. This sounds like > a huge, unnecessary overhead. If we use the visitor pattern, why do we need to collect them (what?) in a vector? The current API looks not nice in a sense that it is leaking V8 internal implementations to an embedder (e.g., mysterious checks in ScriptWrappableHeapTracer::TraceWrappableFrom, retrieving class IDs from ScriptWrappable etc). If the performance overhead really matters, I'm okay with it though. c.f., the current minor GC is using v8::PersistentHandleVisitor.
On 2016/04/18 at 09:41:14, haraken wrote: > On 2016/04/18 09:35:46, jochen wrote: > > wrt vectors: the std::vector is part of the V8 API, so it's technically allowed > > (and using WTF::Vector is not possible in the V8 API). > > > > I don't think it's a good idea to call for every single handle over the API > > boundary. V8 internally has a vector of to be visited objects. We'd now do one > > call per entry, collecting them on the blink side in a vector. This sounds like > > a huge, unnecessary overhead. > > If we use the visitor pattern, why do we need to collect them (what?) in a vector? inside V8, we need to collect the information in a vector (or some other container), as we can't call to blink during marking in general. so at some point, V8 wants to dump a number of objects on Blink. Once we do incremental marking, Blink doesn't take action on every single object, but stores them in a marking deque. I'd like to make this as little overhead as possible. > > The current API looks not nice in a sense that it is leaking V8 internal implementations to an embedder (e.g., mysterious checks in ScriptWrappableHeapTracer::TraceWrappableFrom, retrieving class IDs from ScriptWrappable etc). If the performance overhead really matters, I'm okay with it though. since there can be multiple global handles to one v8 object, and the class ID is part of the global handle, not the object, only blink can do this magic. > > c.f., the current minor GC is using v8::PersistentHandleVisitor.
On 2016/04/18 09:45:06, jochen wrote: > On 2016/04/18 at 09:41:14, haraken wrote: > > On 2016/04/18 09:35:46, jochen wrote: > > > wrt vectors: the std::vector is part of the V8 API, so it's technically > allowed > > > (and using WTF::Vector is not possible in the V8 API). > > > > > > I don't think it's a good idea to call for every single handle over the API > > > boundary. V8 internally has a vector of to be visited objects. We'd now do > one > > > call per entry, collecting them on the blink side in a vector. This sounds > like > > > a huge, unnecessary overhead. > > > > If we use the visitor pattern, why do we need to collect them (what?) in a > vector? > > inside V8, we need to collect the information in a vector (or some other > container), as we can't call to blink during marking in general. > > so at some point, V8 wants to dump a number of objects on Blink. > > Once we do incremental marking, Blink doesn't take action on every single > object, but stores them in a marking deque. > > I'd like to make this as little overhead as possible. > > > > > The current API looks not nice in a sense that it is leaking V8 internal > implementations to an embedder (e.g., mysterious checks in > ScriptWrappableHeapTracer::TraceWrappableFrom, retrieving class IDs from > ScriptWrappable etc). If the performance overhead really matters, I'm okay with > it though. > > > since there can be multiple global handles to one v8 object, and the class ID is > part of the global handle, not the object, only blink can do this magic. > > > > c.f., the current minor GC is using v8::PersistentHandleVisitor. Just want to confirm, but what we want to iterate in ScriptWrappableHeapTracer::TraceWrappableFrom is global handles, not V8 objects, right? And the global handles know class IDs. I'm assuming that the iteration is essentially similar to the PersistentHandleVisitor -- my question is why we need such a different API for PersistentHandleVisitor and TraceWrappableFrom.
On 2016/04/18 at 09:54:17, haraken wrote: > On 2016/04/18 09:45:06, jochen wrote: > > On 2016/04/18 at 09:41:14, haraken wrote: > > > On 2016/04/18 09:35:46, jochen wrote: > > > > wrt vectors: the std::vector is part of the V8 API, so it's technically > > allowed > > > > (and using WTF::Vector is not possible in the V8 API). > > > > > > > > I don't think it's a good idea to call for every single handle over the API > > > > boundary. V8 internally has a vector of to be visited objects. We'd now do > > one > > > > call per entry, collecting them on the blink side in a vector. This sounds > > like > > > > a huge, unnecessary overhead. > > > > > > If we use the visitor pattern, why do we need to collect them (what?) in a > > vector? > > > > inside V8, we need to collect the information in a vector (or some other > > container), as we can't call to blink during marking in general. > > > > so at some point, V8 wants to dump a number of objects on Blink. > > > > Once we do incremental marking, Blink doesn't take action on every single > > object, but stores them in a marking deque. > > > > I'd like to make this as little overhead as possible. > > > > > > > > The current API looks not nice in a sense that it is leaking V8 internal > > implementations to an embedder (e.g., mysterious checks in > > ScriptWrappableHeapTracer::TraceWrappableFrom, retrieving class IDs from > > ScriptWrappable etc). If the performance overhead really matters, I'm okay with > > it though. > > > > > > since there can be multiple global handles to one v8 object, and the class ID is > > part of the global handle, not the object, only blink can do this magic. > > > > > > c.f., the current minor GC is using v8::PersistentHandleVisitor. > > Just want to confirm, but what we want to iterate in ScriptWrappableHeapTracer::TraceWrappableFrom is global handles, not V8 objects, right? And the global handles know class IDs. > > I'm assuming that the iteration is essentially similar to the PersistentHandleVisitor -- my question is why we need such a different API for PersistentHandleVisitor and TraceWrappableFrom. There are no back pointers from V8 objects to global handles. So we actually iterate over V8 objects here. In other words, the difference between PersistentHandleVisitor and TraceWrappableFrom is that the former is used by Blink to iterate over global handles to get to V8 objects, and the latter is used by V8 to tell Blink about V8 objects.
On 2016/04/18 09:58:29, jochen wrote: > On 2016/04/18 at 09:54:17, haraken wrote: > > On 2016/04/18 09:45:06, jochen wrote: > > > On 2016/04/18 at 09:41:14, haraken wrote: > > > > On 2016/04/18 09:35:46, jochen wrote: > > > > > wrt vectors: the std::vector is part of the V8 API, so it's technically > > > allowed > > > > > (and using WTF::Vector is not possible in the V8 API). > > > > > > > > > > I don't think it's a good idea to call for every single handle over the > API > > > > > boundary. V8 internally has a vector of to be visited objects. We'd now > do > > > one > > > > > call per entry, collecting them on the blink side in a vector. This > sounds > > > like > > > > > a huge, unnecessary overhead. > > > > > > > > If we use the visitor pattern, why do we need to collect them (what?) in a > > > vector? > > > > > > inside V8, we need to collect the information in a vector (or some other > > > container), as we can't call to blink during marking in general. > > > > > > so at some point, V8 wants to dump a number of objects on Blink. > > > > > > Once we do incremental marking, Blink doesn't take action on every single > > > object, but stores them in a marking deque. > > > > > > I'd like to make this as little overhead as possible. > > > > > > > > > > > The current API looks not nice in a sense that it is leaking V8 internal > > > implementations to an embedder (e.g., mysterious checks in > > > ScriptWrappableHeapTracer::TraceWrappableFrom, retrieving class IDs from > > > ScriptWrappable etc). If the performance overhead really matters, I'm okay > with > > > it though. > > > > > > > > > since there can be multiple global handles to one v8 object, and the class > ID is > > > part of the global handle, not the object, only blink can do this magic. > > > > > > > > c.f., the current minor GC is using v8::PersistentHandleVisitor. > > > > Just want to confirm, but what we want to iterate in > ScriptWrappableHeapTracer::TraceWrappableFrom is global handles, not V8 objects, > right? And the global handles know class IDs. > > > > I'm assuming that the iteration is essentially similar to the > PersistentHandleVisitor -- my question is why we need such a different API for > PersistentHandleVisitor and TraceWrappableFrom. > > There are no back pointers from V8 objects to global handles. So we actually > iterate over V8 objects here. In other words, the difference between > PersistentHandleVisitor and TraceWrappableFrom is that the former is used by > Blink to iterate over global handles to get to V8 objects, and the latter is > used by V8 to tell Blink about V8 objects. Thanks for the clarification -- makes sense. I'm okay with the std::vector pattern.
On 2016/04/18 10:30:54, haraken wrote:
> On 2016/04/18 09:58:29, jochen wrote:
> > On 2016/04/18 at 09:54:17, haraken wrote:
> > > On 2016/04/18 09:45:06, jochen wrote:
> > > > On 2016/04/18 at 09:41:14, haraken wrote:
> > > > > On 2016/04/18 09:35:46, jochen wrote:
> > > > > > wrt vectors: the std::vector is part of the V8 API, so it's
> technically
> > > > allowed
> > > > > > (and using WTF::Vector is not possible in the V8 API).
> > > > > >
> > > > > > I don't think it's a good idea to call for every single handle over
> the
> > API
> > > > > > boundary. V8 internally has a vector of to be visited objects. We'd
> now
> > do
> > > > one
> > > > > > call per entry, collecting them on the blink side in a vector. This
> > sounds
> > > > like
> > > > > > a huge, unnecessary overhead.
> > > > >
> > > > > If we use the visitor pattern, why do we need to collect them (what?)
in
> a
> > > > vector?
> > > >
> > > > inside V8, we need to collect the information in a vector (or some other
> > > > container), as we can't call to blink during marking in general.
> > > >
> > > > so at some point, V8 wants to dump a number of objects on Blink.
> > > >
> > > > Once we do incremental marking, Blink doesn't take action on every
single
> > > > object, but stores them in a marking deque.
> > > >
> > > > I'd like to make this as little overhead as possible.
> > > >
> > > > >
> > > > > The current API looks not nice in a sense that it is leaking V8
internal
> > > > implementations to an embedder (e.g., mysterious checks in
> > > > ScriptWrappableHeapTracer::TraceWrappableFrom, retrieving class IDs from
> > > > ScriptWrappable etc). If the performance overhead really matters, I'm
okay
> > with
> > > > it though.
> > > >
> > > >
> > > > since there can be multiple global handles to one v8 object, and the
class
> > ID is
> > > > part of the global handle, not the object, only blink can do this magic.
> > > > >
> > > > > c.f., the current minor GC is using v8::PersistentHandleVisitor.
> > >
> > > Just want to confirm, but what we want to iterate in
> > ScriptWrappableHeapTracer::TraceWrappableFrom is global handles, not V8
> objects,
> > right? And the global handles know class IDs.
> > >
> > > I'm assuming that the iteration is essentially similar to the
> > PersistentHandleVisitor -- my question is why we need such a different API
for
> > PersistentHandleVisitor and TraceWrappableFrom.
> >
> > There are no back pointers from V8 objects to global handles. So we actually
> > iterate over V8 objects here. In other words, the difference between
> > PersistentHandleVisitor and TraceWrappableFrom is that the former is used by
> > Blink to iterate over global handles to get to V8 objects, and the latter is
> > used by V8 to tell Blink about V8 objects.
>
> Thanks for the clarification -- makes sense. I'm okay with the std::vector
> pattern.
Or would it be an option to design the API like:
class ScriptWrappableVisitor : public v8::FrontierWrappableVisitor {
void trace(void* object) { // V8 calls the trace method for each item in the
std::vector. All internal checks are encapsulated in the V8 side.
ScriptWrappable* wrappable = reinterpret_cast<ScriptWrappable>(object);
wrappable->traceWrappers(); // If Blink wants to report back the frontier,
Blink can call v8Handle->RegisterExternalReference(), which will update the
marking queue in V8.
}
};
?
On 2016/04/18 at 10:46:10, haraken wrote:
> On 2016/04/18 10:30:54, haraken wrote:
> > On 2016/04/18 09:58:29, jochen wrote:
> > > On 2016/04/18 at 09:54:17, haraken wrote:
> > > > On 2016/04/18 09:45:06, jochen wrote:
> > > > > On 2016/04/18 at 09:41:14, haraken wrote:
> > > > > > On 2016/04/18 09:35:46, jochen wrote:
> > > > > > > wrt vectors: the std::vector is part of the V8 API, so it's
> > technically
> > > > > allowed
> > > > > > > (and using WTF::Vector is not possible in the V8 API).
> > > > > > >
> > > > > > > I don't think it's a good idea to call for every single handle
over
> > the
> > > API
> > > > > > > boundary. V8 internally has a vector of to be visited objects.
We'd
> > now
> > > do
> > > > > one
> > > > > > > call per entry, collecting them on the blink side in a vector.
This
> > > sounds
> > > > > like
> > > > > > > a huge, unnecessary overhead.
> > > > > >
> > > > > > If we use the visitor pattern, why do we need to collect them
(what?) in
> > a
> > > > > vector?
> > > > >
> > > > > inside V8, we need to collect the information in a vector (or some
other
> > > > > container), as we can't call to blink during marking in general.
> > > > >
> > > > > so at some point, V8 wants to dump a number of objects on Blink.
> > > > >
> > > > > Once we do incremental marking, Blink doesn't take action on every
single
> > > > > object, but stores them in a marking deque.
> > > > >
> > > > > I'd like to make this as little overhead as possible.
> > > > >
> > > > > >
> > > > > > The current API looks not nice in a sense that it is leaking V8
internal
> > > > > implementations to an embedder (e.g., mysterious checks in
> > > > > ScriptWrappableHeapTracer::TraceWrappableFrom, retrieving class IDs
from
> > > > > ScriptWrappable etc). If the performance overhead really matters, I'm
okay
> > > with
> > > > > it though.
> > > > >
> > > > >
> > > > > since there can be multiple global handles to one v8 object, and the
class
> > > ID is
> > > > > part of the global handle, not the object, only blink can do this
magic.
> > > > > >
> > > > > > c.f., the current minor GC is using v8::PersistentHandleVisitor.
> > > >
> > > > Just want to confirm, but what we want to iterate in
> > > ScriptWrappableHeapTracer::TraceWrappableFrom is global handles, not V8
> > objects,
> > > right? And the global handles know class IDs.
> > > >
> > > > I'm assuming that the iteration is essentially similar to the
> > > PersistentHandleVisitor -- my question is why we need such a different API
for
> > > PersistentHandleVisitor and TraceWrappableFrom.
> > >
> > > There are no back pointers from V8 objects to global handles. So we
actually
> > > iterate over V8 objects here. In other words, the difference between
> > > PersistentHandleVisitor and TraceWrappableFrom is that the former is used
by
> > > Blink to iterate over global handles to get to V8 objects, and the latter
is
> > > used by V8 to tell Blink about V8 objects.
> >
> > Thanks for the clarification -- makes sense. I'm okay with the std::vector
> > pattern.
>
> Or would it be an option to design the API like:
>
> class ScriptWrappableVisitor : public v8::FrontierWrappableVisitor {
> void trace(void* object) { // V8 calls the trace method for each item in
the std::vector. All internal checks are encapsulated in the V8 side.
> ScriptWrappable* wrappable = reinterpret_cast<ScriptWrappable>(object);
> wrappable->traceWrappers(); // If Blink wants to report back the
frontier, Blink can call v8Handle->RegisterExternalReference(), which will
update the marking queue in V8.
> }
> };
>
> ?
yes, that would be possible, but that would mean one API call per object instead
of one API call in total.
On 2016/04/18 10:47:32, jochen wrote:
> On 2016/04/18 at 10:46:10, haraken wrote:
> > On 2016/04/18 10:30:54, haraken wrote:
> > > On 2016/04/18 09:58:29, jochen wrote:
> > > > On 2016/04/18 at 09:54:17, haraken wrote:
> > > > > On 2016/04/18 09:45:06, jochen wrote:
> > > > > > On 2016/04/18 at 09:41:14, haraken wrote:
> > > > > > > On 2016/04/18 09:35:46, jochen wrote:
> > > > > > > > wrt vectors: the std::vector is part of the V8 API, so it's
> > > technically
> > > > > > allowed
> > > > > > > > (and using WTF::Vector is not possible in the V8 API).
> > > > > > > >
> > > > > > > > I don't think it's a good idea to call for every single handle
> over
> > > the
> > > > API
> > > > > > > > boundary. V8 internally has a vector of to be visited objects.
> We'd
> > > now
> > > > do
> > > > > > one
> > > > > > > > call per entry, collecting them on the blink side in a vector.
> This
> > > > sounds
> > > > > > like
> > > > > > > > a huge, unnecessary overhead.
> > > > > > >
> > > > > > > If we use the visitor pattern, why do we need to collect them
> (what?) in
> > > a
> > > > > > vector?
> > > > > >
> > > > > > inside V8, we need to collect the information in a vector (or some
> other
> > > > > > container), as we can't call to blink during marking in general.
> > > > > >
> > > > > > so at some point, V8 wants to dump a number of objects on Blink.
> > > > > >
> > > > > > Once we do incremental marking, Blink doesn't take action on every
> single
> > > > > > object, but stores them in a marking deque.
> > > > > >
> > > > > > I'd like to make this as little overhead as possible.
> > > > > >
> > > > > > >
> > > > > > > The current API looks not nice in a sense that it is leaking V8
> internal
> > > > > > implementations to an embedder (e.g., mysterious checks in
> > > > > > ScriptWrappableHeapTracer::TraceWrappableFrom, retrieving class IDs
> from
> > > > > > ScriptWrappable etc). If the performance overhead really matters,
I'm
> okay
> > > > with
> > > > > > it though.
> > > > > >
> > > > > >
> > > > > > since there can be multiple global handles to one v8 object, and the
> class
> > > > ID is
> > > > > > part of the global handle, not the object, only blink can do this
> magic.
> > > > > > >
> > > > > > > c.f., the current minor GC is using v8::PersistentHandleVisitor.
> > > > >
> > > > > Just want to confirm, but what we want to iterate in
> > > > ScriptWrappableHeapTracer::TraceWrappableFrom is global handles, not V8
> > > objects,
> > > > right? And the global handles know class IDs.
> > > > >
> > > > > I'm assuming that the iteration is essentially similar to the
> > > > PersistentHandleVisitor -- my question is why we need such a different
API
> for
> > > > PersistentHandleVisitor and TraceWrappableFrom.
> > > >
> > > > There are no back pointers from V8 objects to global handles. So we
> actually
> > > > iterate over V8 objects here. In other words, the difference between
> > > > PersistentHandleVisitor and TraceWrappableFrom is that the former is
used
> by
> > > > Blink to iterate over global handles to get to V8 objects, and the
latter
> is
> > > > used by V8 to tell Blink about V8 objects.
> > >
> > > Thanks for the clarification -- makes sense. I'm okay with the std::vector
> > > pattern.
> >
> > Or would it be an option to design the API like:
> >
> > class ScriptWrappableVisitor : public v8::FrontierWrappableVisitor {
> > void trace(void* object) { // V8 calls the trace method for each item in
> the std::vector. All internal checks are encapsulated in the V8 side.
> > ScriptWrappable* wrappable = reinterpret_cast<ScriptWrappable>(object);
> > wrappable->traceWrappers(); // If Blink wants to report back the
> frontier, Blink can call v8Handle->RegisterExternalReference(), which will
> update the marking queue in V8.
> > }
> > };
> >
> > ?
>
> yes, that would be possible, but that would mean one API call per object
instead
> of one API call in total.
Thanks, I think I'm on the same page.
I'm not sure if the API call overhead really matters (because we're already
doing a bunch of checks in TraceWrappableFrom), but I'll defer the decision to
you.
On 2016/04/18 at 10:52:45, haraken wrote:
> On 2016/04/18 10:47:32, jochen wrote:
> > On 2016/04/18 at 10:46:10, haraken wrote:
> > > On 2016/04/18 10:30:54, haraken wrote:
> > > > On 2016/04/18 09:58:29, jochen wrote:
> > > > > On 2016/04/18 at 09:54:17, haraken wrote:
> > > > > > On 2016/04/18 09:45:06, jochen wrote:
> > > > > > > On 2016/04/18 at 09:41:14, haraken wrote:
> > > > > > > > On 2016/04/18 09:35:46, jochen wrote:
> > > > > > > > > wrt vectors: the std::vector is part of the V8 API, so it's
> > > > technically
> > > > > > > allowed
> > > > > > > > > (and using WTF::Vector is not possible in the V8 API).
> > > > > > > > >
> > > > > > > > > I don't think it's a good idea to call for every single handle
> > over
> > > > the
> > > > > API
> > > > > > > > > boundary. V8 internally has a vector of to be visited objects.
> > We'd
> > > > now
> > > > > do
> > > > > > > one
> > > > > > > > > call per entry, collecting them on the blink side in a vector.
> > This
> > > > > sounds
> > > > > > > like
> > > > > > > > > a huge, unnecessary overhead.
> > > > > > > >
> > > > > > > > If we use the visitor pattern, why do we need to collect them
> > (what?) in
> > > > a
> > > > > > > vector?
> > > > > > >
> > > > > > > inside V8, we need to collect the information in a vector (or some
> > other
> > > > > > > container), as we can't call to blink during marking in general.
> > > > > > >
> > > > > > > so at some point, V8 wants to dump a number of objects on Blink.
> > > > > > >
> > > > > > > Once we do incremental marking, Blink doesn't take action on every
> > single
> > > > > > > object, but stores them in a marking deque.
> > > > > > >
> > > > > > > I'd like to make this as little overhead as possible.
> > > > > > >
> > > > > > > >
> > > > > > > > The current API looks not nice in a sense that it is leaking V8
> > internal
> > > > > > > implementations to an embedder (e.g., mysterious checks in
> > > > > > > ScriptWrappableHeapTracer::TraceWrappableFrom, retrieving class
IDs
> > from
> > > > > > > ScriptWrappable etc). If the performance overhead really matters,
I'm
> > okay
> > > > > with
> > > > > > > it though.
> > > > > > >
> > > > > > >
> > > > > > > since there can be multiple global handles to one v8 object, and
the
> > class
> > > > > ID is
> > > > > > > part of the global handle, not the object, only blink can do this
> > magic.
> > > > > > > >
> > > > > > > > c.f., the current minor GC is using v8::PersistentHandleVisitor.
> > > > > >
> > > > > > Just want to confirm, but what we want to iterate in
> > > > > ScriptWrappableHeapTracer::TraceWrappableFrom is global handles, not
V8
> > > > objects,
> > > > > right? And the global handles know class IDs.
> > > > > >
> > > > > > I'm assuming that the iteration is essentially similar to the
> > > > > PersistentHandleVisitor -- my question is why we need such a different
API
> > for
> > > > > PersistentHandleVisitor and TraceWrappableFrom.
> > > > >
> > > > > There are no back pointers from V8 objects to global handles. So we
> > actually
> > > > > iterate over V8 objects here. In other words, the difference between
> > > > > PersistentHandleVisitor and TraceWrappableFrom is that the former is
used
> > by
> > > > > Blink to iterate over global handles to get to V8 objects, and the
latter
> > is
> > > > > used by V8 to tell Blink about V8 objects.
> > > >
> > > > Thanks for the clarification -- makes sense. I'm okay with the
std::vector
> > > > pattern.
> > >
> > > Or would it be an option to design the API like:
> > >
> > > class ScriptWrappableVisitor : public v8::FrontierWrappableVisitor {
> > > void trace(void* object) { // V8 calls the trace method for each item
in
> > the std::vector. All internal checks are encapsulated in the V8 side.
> > > ScriptWrappable* wrappable =
reinterpret_cast<ScriptWrappable>(object);
> > > wrappable->traceWrappers(); // If Blink wants to report back the
> > frontier, Blink can call v8Handle->RegisterExternalReference(), which will
> > update the marking queue in V8.
> > > }
> > > };
> > >
> > > ?
> >
> > yes, that would be possible, but that would mean one API call per object
instead
> > of one API call in total.
>
> Thanks, I think I'm on the same page.
>
> I'm not sure if the API call overhead really matters (because we're already
doing a bunch of checks in TraceWrappableFrom), but I'll defer the decision to
you.
Marcel, your call then :)
Wonderful, let the poor intern decide :) Ok, I'll keep the vector here. Regarding the checks, there are ways how to get rid of them (down to 1 if). But I'll make sure to profile the difference of both approaches and let you know what the reality is. In case the difference is negligible or absent, I'll revert to one api call per object.
Incorporated the comments, I already can see the finish line, so please bear with me for these last meters :) And ptal :) https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/IDLExtendedAttributes.md (right): https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/IDLExtendedAttributes.md:1284: The generator generates a functions called `XXX::traceWrappers` which is called by `ScriptWrappableHeapTracer` during V8 GC. The function adds references from the XXX instance to this object's element1() and element2() children. Done. https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ActiveScriptWrappable.h (right): https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ActiveScriptWrappable.h:26: static void traceActiveScriptWrappables(ScriptWrappableVisitor*); On 2016/04/18 at 04:35:41, haraken wrote: > traceActiveScriptWrappables => traceWrappers (for consistency) Cannot, sadly. C++ is confused. ActiveScriptWrappables are also ScriptWrappables, so there are then 2 methods called traceWrappers, one instance, one static, and that's ambiguous. https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/DOMDataStore.h (right): https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/DOMDataStore.h:159: scriptWrappable->markWrapper(isolate); On 2016/04/18 at 04:35:41, haraken wrote: > Sorry, this would be a bug of the existing bindings... You'll never this branch because all wrappers in the main world are stored in ScriptWrappables (i.e., doesn't use DOMDataStore at all). > > If you ever hit this branch, I guess you'll run into an infinite tracing loop. > > (I'll remove the m_isMainWorld and make sure that DOMDataStore is used only by isolated worlds.) Yeah, I see. Good catch. I fixed the code. https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/DOMWrapperMap.h (right): https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/DOMWrapperMap.h:98: m_map.RegisterExternallyReferencedObject(object); On 2016/04/18 at 04:35:41, haraken wrote: > I'm okay with this, but it would be nicer if all call sites of RegisterExternallyReferencedObject are unified into ScriptWrappableHeapTracer::markWrapper. > > If you cannot do the unification, you can keep this, but then it would make more sense to remove ScriptWrappableHeapTracer::markWrapper. Alternately, ScriptWrappable::markWrapper can directly call RegisterExternallyReferencedObject. (i.e., ScriptWrappable takes care of the main world, and DOMWrapperMap takes care of isolated worlds.) I don't think we can unify this elegantly, the map lives in the V8, it's weird to make it give us information just so we can pass it back. I propose following: ScriptWrappableHeapTracer is THE place to use to register external references for the v8, like a bottleneck. It has 2 markWrapper-ish methods: one taking ScriptWrappables (called markWrappersInAllWorlds)- and marking wrappers in all worlds, second one taking v8::persistent (called markWrapper) - and marking only the given wrapper. Heap tracer for markWrappersInAllWorlds, behind the scenes, delegates to ScriptWrappable.markWrapper for main world and to DOMDataStore and that to DOMWrapperMap for other worlds. I agree it's mess, I'm still waiting for Muse to come and enlighten me... At least I added the comments to reveal our intentions more. https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptWrappable.cpp (right): https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptWrappable.cpp:39: void ScriptWrappable::markWrapper(v8::Isolate* isolate) const On 2016/04/18 at 04:35:41, haraken wrote: > This method looks redundant. The caller sites of this method should just call ScriptWrappableHeapTracer::markWrapper. > > (Below I'm suggesting renaming it to ScriptWrappableHeapTracer::markWrappersInAllWorlds.) I agree, in this form the method is useless. I changed it so it actually marks its wrapper, if contains one. See the comment above for longer discussion. https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.cpp (right): https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.cpp:32: void ScriptWrappableHeapTracer::ClearTracingMarks(v8::Isolate* isolate) On 2016/04/18 at 04:35:42, haraken wrote: > TraceRoots => TracePrologue > ClearTracingMarks => TraceEpilogue > > ? I like it, I uploaded cl for the v8 side, after landing I'll change this one accordingly. https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.cpp:41: void ScriptWrappableHeapTracer::TraceWrappableFrom(v8::Isolate* isolate, const std::vector<std::pair<void*, void*>>& internalFieldsOfPotentialWrappers) On 2016/04/18 at 04:35:42, haraken wrote: > std::vector is not allowed in Blink. > > Can we use a visitor pattern (e.g., v8::PersistentHandleVisitor) and avoid passing std::vector to Blink? Leaving as is after the discussion. https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.cpp:55: ASSERT_NOT_REACHED(); On 2016/04/18 at 04:35:42, haraken wrote: > Then we can encapsulate this mysterious check in the V8 side. Agreed, moved. https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.cpp:63: if (classId != WrapperTypeInfo::NodeClassId On 2016/04/18 at 04:35:41, haraken wrote: > Actually I want to ask V8 to pass in the class ID to Blink so that Blink just needs to check the class ID. > > In other words, I want to design the ScriptWrappableHeapTracer in the same manner as the v8::PersistentHandleVisitor. I'm afraid that getting class id from a wrapper is non trivial and "not worth it" (I'm not hitting global handles at all, and class id is a field of GlobalHandles::Node). https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.cpp:92: void ScriptWrappableHeapTracer::markWrapper(const ScriptWrappable* scriptWrappable, v8::Isolate* isolate) Done. https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.cpp:97: DOMWrapperWorld::visitAllWorldsInMainThread(visitor); Agreed, updated the code accordingly. https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.cpp:104: dataStore.markWrapper(m_scriptWrappable, m_isolate); On 2016/04/18 at 04:35:41, haraken wrote: > And you can call this in DOMWrapperWorld::markWrappersInAllWorlds(ScriptWrappable*). > > Also add a TODO and mention that we should avoid tracing all wrappers in isolated worlds. (Actually I'm a bit afraid of the performance overhead but let's worry about the details later :-) Fixed and TODO added. I worry about the performance too, tracing worlds individually is for sure on the roadmap. https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.h (right): https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.h:47: std::vector<HeapObjectHeader*> m_headersToUnmark; Done. https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp (right): https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp:43: void ScriptWrappableVisitor::traceWrappers(const ScriptWrappable& wrappable) const On 2016/04/18 at 04:35:42, haraken wrote: > Is this method needed? Will be. https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h (right): https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h:49: DECLARE_VIRTUAL_TRACE_WRAPPERS() override; On 2016/04/18 at 04:35:42, haraken wrote: > Yeah, what I was suggesting is to remove OVERRIDE_TRACE_WRAPPERS() and just use DECLARE_VIRTUAL_TRACE_WRAPPERS(). > > c.f., Oilpan uses DECLARE_VIRTUAL_TRACE() for both parent and child classes. That would mean skipping override keyword. Are you fine with that? https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h:83: bool isMarked(const void* garbageCollected) const; On 2016/04/18 at 04:35:42, haraken wrote: > mark => markWrapper > isMarked => isWrapperMarked To be honest, I don't like the word wrapper here, we are not marking wrapper, we mark the header in the script wrappable. I renamed it to markHeader, so it's not confused with oilpan visiting. https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/tests/idls/core/TestTraceWrapperReferences.idl (right): https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/tests/idls/core/TestTraceWrapperReferences.idl:6: TraceWrapperReferences=(firstChild,nextSibling,prevSibling,parent) Done. https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/NodeRareData.cpp (right): https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/NodeRareData.cpp:68: DEFINE_TRACE_WRAPPERS(NodeRareData) Removed. https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/NodeRareData.h (right): https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/NodeRareData.h:105: DECLARE_NONVIRTUAL_TRACE_WRAPPERS(); Ok. Removed. https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/HeapPage.h (right): https://codereview.chromium.org/1876383003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/HeapPage.h:211: bool isMarked() const; Done.
Refactored to use new v8 api (only renamed methods)
LGTM All comments are just nits. https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/IDLExtendedAttributes.txt (right): https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/IDLExtendedAttributes.txt:91: TraceWrapperReferences=* TraceWrapperReferences => TraceWrappers ? https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/DOMDataStore.h (right): https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/DOMDataStore.h:156: void markWrapper(ScriptWrappable* scriptWrappable, v8::Isolate* isolate) Remove the isolate parameter. https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (right): https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:146: ASSERT(isMainThread()); How is it guaranteed that this function is not called in worker threads? https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:151: DOMDataStore& dataStore = it->value->domDataStore(); You can use: for (auto& isolatedWorld : isolatedWorlds.values()) https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.cpp (right): https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.cpp:59: if (classId != WrapperTypeInfo::NodeClassId ASSERT(classId == WrapperTypeInfo::NodeClassId || classId -= WrapperTypeInfo::ObjectClassId); https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.cpp:69: void ScriptWrappableHeapTracer::TraceWrappersFrom(v8::Isolate* isolate, v8::Persistent<v8::Object>* handle) Where is this method used? https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.cpp:74: && classId != WrapperTypeInfo::ObjectClassId) { Ditto. https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.h (right): https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.h:28: void AddHeaderToUnmark(HeapObjectHeader*); AddHeaderToUnmark => addHeaderToUnmark https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.h:40: void TraceWrappersFrom(v8::Isolate*, std::pair<void*, void*> internalFields); TraceWrappersFrom => traceWrappersFrom https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp (right): https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp:16: bool ScriptWrappableVisitor::isHeaderMarked(const void* garbageCollected) const If you think that the tracing performance really matters enough to optimize the API call overhead, I think you should inline all the methods of ScriptWrappableVisitor. https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp:31: header->markWrapper(); It looks a bit confusing that markHeader calls markWrapper. I'd suggest renaming all the methods to markWrapperHeader consistently. https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h (right): https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h:30: #define DECLARE_NONVIRTUAL_TRACE_WRAPPERS() \ DECLARE_NONVIRTUAL_TRACE_WRAPPERS => DECLARE_TRACE_WRAPPERS (to make the macro consistent with Oilpan's one) https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h:48: #define OVERRIDE_TRACE_WRAPPERS() \ Yeah, I'd prefer dropping OVERRIDE in order to make the macros consistent with Oilpan's ones. https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h:70: class ScriptWrappableVisitor { I guess you can just remove ScriptWrappableVisitor and move the contents to ScriptWrappableHeapTracer. It looks redundant to create a new ScriptWrappableVisitor every time you trace one ScriptWrappable object. You can just use the ScriptWrappableHeapTracer (which is a singleton). Nit: Then I'll rename ScriptWrappableHeapTracer to ScriptWrappableVisitor. https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/scripts/v8_interface.py (right): https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/v8_interface.py:177: 'trace_wrapper_references': trace_wrapper_references, trace_wrapper_references => trace_wrappers https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/HeapPage.h (right): https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/HeapPage.h:210: void unmarkWrapper(); isWrapperHeaderMarked markWrapperHeader unmarkWrapperHeader
awesome Marcel! https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.h (right): https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.h:17: * When this heap tracer is set to the v8::Isolate, the V8 will call it during remove the in "the V8" also below and in other files. https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/HeapPage.h (right): https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/HeapPage.h:145: const size_t headerWrapperMarkBitMast = 1u << 17; headerWrapperMarkBitMask
Final version. After the v8 changes reach chromium repo, and there aren't any objections, I will commit this cl. Thank you so much! https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/IDLExtendedAttributes.txt (right): https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/IDLExtendedAttributes.txt:91: TraceWrapperReferences=* Done. https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/DOMDataStore.h (right): https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/DOMDataStore.h:156: void markWrapper(ScriptWrappable* scriptWrappable, v8::Isolate* isolate) Done. https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (right): https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:146: ASSERT(isMainThread()); On 2016/04/19 at 04:58:30, haraken wrote: > How is it guaranteed that this function is not called in worker threads? I changed the assert to if (...) return. https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:151: DOMDataStore& dataStore = it->value->domDataStore(); Done. https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.cpp (right): https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.cpp:59: if (classId != WrapperTypeInfo::NodeClassId Done. https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.cpp:69: void ScriptWrappableHeapTracer::TraceWrappersFrom(v8::Isolate* isolate, v8::Persistent<v8::Object>* handle) On 2016/04/19 at 04:58:31, haraken wrote: > Where is this method used? Nowhere, removed. https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.cpp:74: && classId != WrapperTypeInfo::ObjectClassId) { Done. https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.h (right): https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.h:17: * When this heap tracer is set to the v8::Isolate, the V8 will call it during Done. https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.h:28: void AddHeaderToUnmark(HeapObjectHeader*); Done. https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableHeapTracer.h:40: void TraceWrappersFrom(v8::Isolate*, std::pair<void*, void*> internalFields); Done. https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp (right): https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp:16: bool ScriptWrappableVisitor::isHeaderMarked(const void* garbageCollected) const Done for private methods. I'll measure the impact of both api call optimization and inlining after we're correctly tracing all and refactor code to inline traceWrappers too if neccessary. https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp:31: header->markWrapper(); Done. https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h (right): https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h:30: #define DECLARE_NONVIRTUAL_TRACE_WRAPPERS() \ Done. https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h:48: #define OVERRIDE_TRACE_WRAPPERS() \ Done. https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h:70: class ScriptWrappableVisitor { On 2016/04/19 at 04:58:31, haraken wrote: > I guess you can just remove ScriptWrappableVisitor and move the contents to ScriptWrappableHeapTracer. It looks redundant to create a new ScriptWrappableVisitor every time you trace one ScriptWrappable object. You can just use the ScriptWrappableHeapTracer (which is a singleton). > > Nit: Then I'll rename ScriptWrappableHeapTracer to ScriptWrappableVisitor. Done. I cleaned up v8 api once again. https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/scripts/v8_interface.py (right): https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/v8_interface.py:177: 'trace_wrapper_references': trace_wrapper_references, Done. https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/HeapPage.h (right): https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/HeapPage.h:145: const size_t headerWrapperMarkBitMast = 1u << 17; Done. https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/HeapPage.h:210: void unmarkWrapper(); Done. https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/HeapPage.h:210: void unmarkWrapper(); Done.
Still LGTM https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp (right): https://codereview.chromium.org/1876383003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp:16: bool ScriptWrappableVisitor::isHeaderMarked(const void* garbageCollected) const On 2016/04/19 12:40:29, Marcel Hlopko wrote: > Done for private methods. I'll measure the impact of both api call optimization > and inlining after we're correctly tracing all and refactor code to inline > traceWrappers too if neccessary. Sounds like a plan :)
The CQ bit was checked by hlopko@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1876383003/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1876383003/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by hlopko@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1876383003/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1876383003/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_android on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by hlopko@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1876383003/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1876383003/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by hlopko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org Link to the patchset: https://codereview.chromium.org/1876383003/#ps240001 (title: "Fix var used only in assert")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1876383003/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1876383003/240001
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
machenbach@chromium.org changed reviewers: + hablich@chromium.org, machenbach@chromium.org
Message was sent while issue was closed.
FYI: Please keep a bigger window between v8 api changes and chromium patches: https://build.chromium.org/p/chromium.fyi/builders/Linux%20V8%20API%20Stabili... Otherwise it's harder to revert v8 for canaries.
Message was sent while issue was closed.
Will do, sorry. On Thu, Apr 21, 2016 at 9:51 AM <machenbach@chromium.org> wrote: > FYI: Please keep a bigger window between v8 api changes and chromium > patches: > > https://build.chromium.org/p/chromium.fyi/builders/Linux%20V8%20API%20Stabili... > > Otherwise it's harder to revert v8 for canaries. > > https://codereview.chromium.org/1876383003/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
Will do, sorry. On Thu, Apr 21, 2016 at 9:51 AM <machenbach@chromium.org> wrote: > FYI: Please keep a bigger window between v8 api changes and chromium > patches: > > https://build.chromium.org/p/chromium.fyi/builders/Linux%20V8%20API%20Stabili... > > Otherwise it's harder to revert v8 for canaries. > > https://codereview.chromium.org/1876383003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
Description was changed from ========== Introduce infrastructure for tracing ScriptWrappables. This cl introduces components needed to trace ScriptWrappable references. Unsurprisingly, this cl does not aim for correctness and is far from tracing all the wrappables. Instead it demonstrates the approach we've chosen, and will foster the discussion about future direction. Once we polish the details, I will upload cls which will improve the tracing coverage. We introduced TraceWrappableReferences idl annotation, which will generate the tracing method. The intended usage is demonstrated on Node class. Not all objects we have to trace through are ScriptWrappable instances, and the intended solution to this problem is shown on NodeRareData class. Comments are more than welcome! BUG=468240 LOG=no ========== to ========== Introduce infrastructure for tracing ScriptWrappables. This cl introduces components needed to trace ScriptWrappable references. Unsurprisingly, this cl does not aim for correctness and is far from tracing all the wrappables. Instead it demonstrates the approach we've chosen, and will foster the discussion about future direction. Once we polish the details, I will upload cls which will improve the tracing coverage. We introduced TraceWrappableReferences idl annotation, which will generate the tracing method. The intended usage is demonstrated on Node class. Not all objects we have to trace through are ScriptWrappable instances, and the intended solution to this problem is shown on NodeRareData class. Comments are more than welcome! BUG=468240 LOG=no Committed: https://crrev.com/47fcf60157c8e5e4898e8fc4eeafbe5a92159589 Cr-Commit-Position: refs/heads/master@{#388506} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/47fcf60157c8e5e4898e8fc4eeafbe5a92159589 Cr-Commit-Position: refs/heads/master@{#388506} |
