After some thought I agree that this can live in gin. But how to expose it from there is not immediately obvious.
I've added an additional enum value to gin's GinEmbedder: kEmbedderUnknown. (This enum seems to be used both for the wrapper tracing, and as an offset into the isolate's data slots. Perhaps for tracing a new enum should be created?) I am adding a new public class to gin: MultiHeapTracer. This is a v8::EmbedderHeapTracer, but it has two additional methods: AddHeapTracer and RemoveHeapTracer. The former takes a v8::EmbedderHeapTracer and GinEmbedder. Internally it has a map of embedder -> v8::EmbedderHeapTracer. If you specify kEmbedderUnknown in the call to AddTracer (it's a default parameter, so more like if you don't specify), then it will generate a new id that you can use for the embedder field of the WrapperInfo struct for wrappers that tracer is supposed to trace. I added an instance of this MultiHeapTracer to gin's PerIsolateData, and added a method to gin's IsolateHolder to provide access to the HeapTracer held in the isolate data. Then within blink's code, the V8PerIsolateData creates its ScriptWrappableMarkingVisitor, and sets it on the heap tracer, gaining access via the IsolateHolder it has as a data member. The problem is how do we access this from outside of blink/gin private code so that embedders can add their own tracer? Neither gin::PerIsolateData or blink::V8PerIsolateData are public. Thanks, Daryl. On Friday, 15 June 2018 04:53:15 UTC-4, Jochen Eisinger wrote: > > I guess what I'm saying is not that you should use gin, but that we > shouldn't change chromium to support multiple tracers, as I don't want to > have to support code that doesn't go through gin > > On Fri, Jun 15, 2018 at 7:35 AM Michael Lippautz <mlip...@chromium.org > <javascript:>> wrote: > >> I am OOO until beginning of July now. >> >> Overall, I think we can add that to v8, although I think it would still >> fit better in gin as we will always need to have some WrapperTypeInfo to >> match against when used in the context of Chromium. >> >> On Fri, Jun 15, 2018 at 12:27 AM 'Daryl Haresign' via v8-dev < >> v8-...@googlegroups.com <javascript:>> wrote: >> >>> For some reason my replies seem to be automatically deleted :( >>> >>> On Thursday, 14 June 2018 03:17:05 UTC-4, Jochen Eisinger wrote: >>>> >>>> I'd rather move tracing functionality into gin than add more required >>>> embedder fields. Adding the tracing to gin might also be nice once we move >>>> PDFium to use gin bindings. >>>> >>> >>> If this was added to gin, how do you see it working? The changes I >>> proposed to v8 could be put in gin instead, but we'd still have to come up >>> with a way to get Chromium to ignore them. Even with the changes solely in >>> v8, we could have a type in our library which matches gin's WrapperInfo, >>> and have an embedder field with a value that doesn't match a value in >>> GinEmbedders. With this, Chromium wouldn't need any changes as it >>> already checks for kEmbedderBlink. It would be nicer if this >>> WrapperInfo type was in v8 though. >>> >>> I think checking out gin in addition to v8 is a negligible cost, and I'd >>>> much rather have embedders go through gin than trying to write their own >>>> bindings. >>>> >>> >>> Again, our bindings work in environments that both have and don't have >>> Chromium, so gin isn't a dependency we really want to introduce to our >>> bindings. >>> >>> On Wed, Jun 13, 2018 at 7:04 PM d.haresign via v8-dev < >>>> v8-...@googlegroups.com> wrote: >>>> >>>>> On Tuesday, 12 June 2018 22:15:01 UTC-4, mlip...@chromium.org wrote: >>>>>> >>>>>> +Jochen Eisinger who is gin >>>>>> <https://cs.chromium.org/chromium/src/gin/?g=0> owner and also has a >>>>>> good overview of the whole Chromium architecture. >>>>>> >>>>>> The Chromium dispatch for the EmbedderHeapTracer currently needs two >>>>>> internal embedder fields and no lookup for a tracer. It requires a >>>>>> suboptimal pattern match on embedder fields that we should optimize >>>>>> though. >>>>>> >>>>>> I am happy to discuss any improvements on that, especially the >>>>>> matching for whether we should send or not, but the performance for >>>>>> using >>>>>> it in Chromium should stay similar as this is used everywhere for the >>>>>> DOM >>>>>> and Blink is the main embedder. >>>>>> >>>>> >>>>> Understood. You mentioned having a bit somewhere in the hidden class, >>>>> can you elaborate a bit on that? I'd be happy to investigate doing this >>>>> instead. >>>>> >>>>> I think that if a project wants to live as an embedder in the >>>>>> Chromium/Blink environment, then it should at least depend on gin. We >>>>>> can >>>>>> then think of adding maybe adding a the dispatcher to gin that then >>>>>> forwards based on the GinEmbedder >>>>>> <https://cs.chromium.org/chromium/src/gin/public/gin_embedders.h?q=GinEmbedder&g=0&l=14> >>>>>> and >>>>>> set that to V8 to generalize this for the Chromium world. Jochen, wdyt? >>>>>> >>>>> >>>>> I have two concerns about this. First, from what I can tell, gin >>>>> seems to be part of Chromium, at least in terms of the source >>>>> distribution, >>>>> so we'd effectively be depending on Chromium. But second and more >>>>> importantly, we don't always embed alongside Chromium. Our library is >>>>> also >>>>> used on the server-side. For that reason we'd rather just depend on V8 >>>>> and >>>>> have the same code work in both environments. >>>>> >>>>> Whilst you can put the dispatching tracer somewhere other than V8, I'm >>>>> don't think it's unreasonable to have the logic in V8 itself. >>>>> >>>>> Let me enumerate the changes I think are required in V8: >>>>> >>>>> - Add AddEmbedderHeapTracer and RemoveEmbedderHeapTracer >>>>> - Deprecate SetEmbedderHeapTracer (make it forward to >>>>> AddEmbedderHeapTracer) >>>>> - LocalEmbedderHeapTracer to store container of remote tracers >>>>> - LocalEmbedderHeapTracer to call all tracers in a loop >>>>> - NumberOfWrappersToTrace needs to take a sum >>>>> - AdvanceTracing needs some thought: does it attempt to finish >>>>> one remote tracer before moving on to the next, or round-robin? >>>>> - Everything else can just iterate and call >>>>> >>>>> >> These changes look fine except that we pull in the decision on how to >> schedule AdvanceTracing when multiple embedders are involved which is >> something I don't like. We would probably just preserve the existing step >> size and go round robin. >> >> We cache wrappers and only push them over in batches so the iteration >> over the tracers should not matter. >> >> >> >>> That's basically it. However there are potentially more changes (e.g. >>>>> to TracePossibleWrappers) when you consider how a third party tracer >>>>> would work in conjunction with Chromium's. >>>>> >>>>> I envisaged that V8 would simply tell all remote tracers about all >>>>> wrappers. Each tracer would be responsible for filtering out the >>>>> wrappers >>>>> that don't belong to it. If there was a "contract" that the first >>>>> embedder >>>>> field was set to point to the tracer to be used, then the implementation >>>>> would be dead simple: >>>>> >>>>> void ScriptWrappableMarkingVisitor::RegisterV8References( >>>>> const std::vector<std::pair<void *, void *>>& >>>>> internal_fields_of_potential_wrappers) { >>>>> CHECK(ThreadState::Current()); >>>>> for (auto& pair : internal_fields_of_potential_wrappers) { >>>>> if (pair.first == this) { >>>>> RegisterV8Reference(pair); >>>>> } >>>>> } >>>>> } >>>>> >>>>> But there are other options on how to do this, if adding a third field >>>>> would be an unacceptable cost. We could adjust V8's >>>>> TracePossibleWrappers >>>>> to not check if the first field is nullptr, and adjust Chromium's >>>>> RegisterV8References >>>>> to filter out nullptrs. We can probably come up with a scheme for >>>>> our tracer that works with only one field (though it would be nicer if we >>>>> had access to more than just the first two...) >>>>> >>>>> >> I would like to avoid eagerly sending an object that will anyways be >> filtered by Blink when no other embedder is involved. >> >> I don't think gin is a huge dependency but you can probably just mirror >> WrapperTypeInfo in the sense that the enums match like suggested. We can >> add the filter on the Blink side that matches against kEmbedderBlink so >> that we only handle the right objects. >> >> Less fields is obviously better and I think by now it should be possible >> to get around with the two we send. >> >> -Michael >> >> -- >> -- >> v8-dev mailing list >> v8-...@googlegroups.com <javascript:> >> http://groups.google.com/group/v8-dev >> --- >> You received this message because you are subscribed to the Google Groups >> "v8-dev" group. >> To unsubscribe from this group and stop receiving emails from it, send an >> email to v8-dev+un...@googlegroups.com <javascript:>. >> For more options, visit https://groups.google.com/d/optout. >> > -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
