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-dev@googlegroups.com> 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-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.
