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 <[email protected]> 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 < > [email protected]> 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 < >>> [email protected]> wrote: >>> >>>> On Tuesday, 12 June 2018 22:15:01 UTC-4, [email protected] 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 > [email protected] > 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 [email protected]. > For more options, visit https://groups.google.com/d/optout. > -- -- v8-dev mailing list [email protected] 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 [email protected]. For more options, visit https://groups.google.com/d/optout.
