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 <javascript:>> 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 >> >> 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...) >> >> Thoughts? >> Daryl. >> >> -- >> -- >> 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.
