Re: [v8-dev] Is Multiple 'EmbedderHeapTracer's Feasible?

['Daryl Haresign' via v8-dev]

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  > 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 > 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 
>>  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 
>> 
>>  and 
>> set that to V8 to generalize this for the Chromium world. Jochen, wdyt?

Re: [v8-dev] Is Multiple 'EmbedderHeapTracer's Feasible?

[Jochen Eisinger]

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 
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-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
>  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
> 
>  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
 TracePos

Re: [v8-dev] Is Multiple 'EmbedderHeapTracer's Feasible?

[Michael Lippautz]

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
  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
 
  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

Re: [v8-dev] Is Multiple 'EmbedderHeapTracer's Feasible?

['Daryl Haresign' via v8-dev]

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 
>>>  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 
>>> 
>>>  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>&
>> 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 tha

Re: [v8-dev] Is Multiple 'EmbedderHeapTracer's Feasible?

[d.haresign via v8-dev]

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.
>

More embedder fields is not a necessity, and having the ability to have 
multiple traces in v8 doesn’t preclude you from having tracing in gin, 
infact it enables it.
 

> 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.
>

We already have a very well established binding system. I don’t think we 
want to rewrite all our code to use gin.
 

> 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 
>>>  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 
>>> 
>>>  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>&
>> 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 
>> 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...@googleg

Re: [v8-dev] Is Multiple 'EmbedderHeapTracer's Feasible?

[Jochen Eisinger]

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.

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.

On Wed, Jun 13, 2018 at 7:04 PM d.haresign via v8-dev <
v8-dev@googlegroups.com> wrote:

> On Tuesday, 12 June 2018 22:15:01 UTC-4, mlip...@chromium.org wrote:
>>
>> +Jochen Eisinger who is gin
>>  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
>> 
>>  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>&
> 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-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.
>

-- 
-- 
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.

Re: [v8-dev] Is Multiple 'EmbedderHeapTracer's Feasible?

[d.haresign via v8-dev]

On Tuesday, 12 June 2018 22:15:01 UTC-4, mlip...@chromium.org wrote:
>
> +Jochen Eisinger who is gin 
>  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 
> 
>  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>&
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-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.

Re: [v8-dev] Is Multiple 'EmbedderHeapTracer's Feasible?

[Michael Lippautz]

+Jochen Eisinger  who is gin
 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.

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

and
set that to V8 to generalize this for the Chromium world. Jochen, wdyt?

-Michael

On Wed, Jun 13, 2018 at 1:21 AM d.haresign via v8-dev <
v8-dev@googlegroups.com> wrote:

> On Tuesday, 12 June 2018 04:52:13 UTC-4, mlip...@chromium.org wrote:
>>
>> On Tue, Jun 12, 2018 at 8:02 AM d.haresign via v8-dev <
>> v8-...@googlegroups.com> wrote:
>>
>>> I work on a product which embeds Chromium.  In addition, we host complex
>>> C++ objects in V8 directly.  Our V8 binding layer supports the tracing of
>>> objects to determine references to other objects, rather than simply
>>> holding on to all objects with strong 'v8::Persistent's.  Up until Chromium
>>> 56 / V8 5.6, we were doing this tracing in the GC prologue, using
>>> 'Isolate::SetReference' to inform V8 of all the edges.
>>>
>>> As we look to upgrade past Chromium 59 / V8 5.9, 'Isolate::SetReference'
>>> has been removed in favor of the 'EmbedderHeapTracer' APIs.
>>>
>>> One issue we have with the switch, is that Chromium already sets the
>>> 'EmbedderHeapTracer' to its own 'ScriptWrappableVisitor' tracer.  In order
>>> to work we need to come up with a way to work along side Chromium.
>>>
>>> To quickly recap on how the tracing works, when an embedder hosts an
>>> object it can set internal fields.  When V8 is tracing objects, if it
>>> discovers an object which has the first and second internal fields both
>>> set, it considers it as a wrapper that needs to be traced with the
>>> 'EmbedderHeapTracer'.  These internal fields let you store information
>>> necessary to perform the tracing.
>>>
>>> In Chromium, the first field is a pointer to a 'WrapperTypeInfo' object,
>>> and the second field is a pointer to a 'ScriptWrappable' object.  The
>>> 'RegisterV8References' implementation in Chromium's tracer expects to only
>>> ever be called with these Chromium objects.  As such, it does a
>>> reinterpret_cast to a 'WrapperTypeInfo *' on the first field.  If the
>>> 'gin_embedder' field of that object is 'kEmbedderBlink', it does a
>>> reinterpret_cast to a 'ScriptWrappable *', and traces that object.
>>>
>>> If we were to integrate with Chromium's 'EmbedderHeapTracer', we would
>>> have to set up our objects to have objects which match the layout of
>>> Chromium's 'WrapperTypeInfo', pretend that the are 'kEmbedderBlink'
>>> objects, and somehow get it to call our trace methods instead of Chromiums
>>> (the exact way this is done seems to vary wildly over the latest 10 or so
>>> versions of Chromium).
>>>
>>> This seems like a rabbit hole full of issues I'd rather avoid.
>>>
>>
>> Just to acknowledge: All of that is correct.
>>
>> Now, you could still do something differently:
>> - Create a generic dispatcher class inheriting from EmbedderHeapTracer.
>> - Instead of setting ScriptWrappableVisitor, pass on the generic
>> dispatcher to V8.
>> - Have your objects match this code
>> 
>> that decides whether something should be passed on or not to the
>> EmbedderHeapTracer. In the case that these objects are not critical wrt. to
>> memory consumption, tge simplest thing would be to have 3 fields of which
>> the first 2 are nullptr.
>> - Have your generic dispatcher use Chromium's ScriptWrappable visitor if
>> it's a Blink object
>> - ... or dispatch to whatever EmbedderHeapTracer, otherwise.
>>
>> The fragile part is the matching algorithm in V8 which we wanted to
>> change but ultimately never needed to. You don't have to deal with
>> WrapperTypeInfo or any of the types specified there as the generic visitor
>> would just pass on the object.
>>
>
> Yeah, that's another option we considered.  Though currently we don't have
> any control over this as Chromium sets its own tracer.  We could change
> Chromium to install a dispatching tracer, and have APIs to let us register
> our own tracer with Chromium's dispatching tracer, but I think we'd prefer
> to keep our V8 binding library working purely in terms of V8, rather than
> adding Chromium into the mix (eve

Re: [v8-dev] Is Multiple 'EmbedderHeapTracer's Feasible?

[d.haresign via v8-dev]

On Tuesday, 12 June 2018 04:52:13 UTC-4, mlip...@chromium.org wrote:
>
> On Tue, Jun 12, 2018 at 8:02 AM d.haresign via v8-dev <
> v8-...@googlegroups.com > wrote:
>
>> I work on a product which embeds Chromium.  In addition, we host complex 
>> C++ objects in V8 directly.  Our V8 binding layer supports the tracing of 
>> objects to determine references to other objects, rather than simply 
>> holding on to all objects with strong 'v8::Persistent's.  Up until Chromium 
>> 56 / V8 5.6, we were doing this tracing in the GC prologue, using 
>> 'Isolate::SetReference' to inform V8 of all the edges.
>>
>> As we look to upgrade past Chromium 59 / V8 5.9, 'Isolate::SetReference' 
>> has been removed in favor of the 'EmbedderHeapTracer' APIs.
>>
>> One issue we have with the switch, is that Chromium already sets the 
>> 'EmbedderHeapTracer' to its own 'ScriptWrappableVisitor' tracer.  In order 
>> to work we need to come up with a way to work along side Chromium.
>>
>> To quickly recap on how the tracing works, when an embedder hosts an 
>> object it can set internal fields.  When V8 is tracing objects, if it 
>> discovers an object which has the first and second internal fields both 
>> set, it considers it as a wrapper that needs to be traced with the 
>> 'EmbedderHeapTracer'.  These internal fields let you store information 
>> necessary to perform the tracing.
>>
>> In Chromium, the first field is a pointer to a 'WrapperTypeInfo' object, 
>> and the second field is a pointer to a 'ScriptWrappable' object.  The 
>> 'RegisterV8References' implementation in Chromium's tracer expects to only 
>> ever be called with these Chromium objects.  As such, it does a 
>> reinterpret_cast to a 'WrapperTypeInfo *' on the first field.  If the 
>> 'gin_embedder' field of that object is 'kEmbedderBlink', it does a 
>> reinterpret_cast to a 'ScriptWrappable *', and traces that object.
>>
>> If we were to integrate with Chromium's 'EmbedderHeapTracer', we would 
>> have to set up our objects to have objects which match the layout of 
>> Chromium's 'WrapperTypeInfo', pretend that the are 'kEmbedderBlink' 
>> objects, and somehow get it to call our trace methods instead of Chromiums 
>> (the exact way this is done seems to vary wildly over the latest 10 or so 
>> versions of Chromium).
>>
>> This seems like a rabbit hole full of issues I'd rather avoid.
>>
>
> Just to acknowledge: All of that is correct.
>
> Now, you could still do something differently:
> - Create a generic dispatcher class inheriting from EmbedderHeapTracer. 
> - Instead of setting ScriptWrappableVisitor, pass on the generic 
> dispatcher to V8.
> - Have your objects match this code 
> 
>  
> that decides whether something should be passed on or not to the 
> EmbedderHeapTracer. In the case that these objects are not critical wrt. to 
> memory consumption, tge simplest thing would be to have 3 fields of which 
> the first 2 are nullptr.
> - Have your generic dispatcher use Chromium's ScriptWrappable visitor if 
> it's a Blink object
> - ... or dispatch to whatever EmbedderHeapTracer, otherwise.
>
> The fragile part is the matching algorithm in V8 which we wanted to change 
> but ultimately never needed to. You don't have to deal with WrapperTypeInfo 
> or any of the types specified there as the generic visitor would just pass 
> on the object.
>

Yeah, that's another option we considered.  Though currently we don't have 
any control over this as Chromium sets its own tracer.  We could change 
Chromium to install a dispatching tracer, and have APIs to let us register 
our own tracer with Chromium's dispatching tracer, but I think we'd prefer 
to keep our V8 binding library working purely in terms of V8, rather than 
adding Chromium into the mix (even if it's linked in elsewhere in the 
program).  We could also expose an API on V8 to fetch the current tracer 
which would allow us to overwrite it with a dispatching tracer, though that 
seems more fragile.

fyi: We were thinking of adding a bit somewhere on V8's hidden class that 
> is set through instantiating it through the template info. That bit 
> could've been used to signal that the object should be sent to the 
> EmbedderHeapTracer instead of doing the pattern matching on fields.
>
 
That sounds good.  Out of interest, is there any reason why the tracer only 
gets the first two embedder fields?  Could it be passed a 'v8::Object' so 
that the tracer could access additional fields, or would there be lifetime 
concerns?  Prior to us needing to use the 'EmbedderHeapTracer' APIs, we 
were using the 4th field when embedding with Chromium, which claims the 
first three.

The way our tracer works when we're not in a process that links in 
Chromium, is that the first embedder field is a pointer to the tracer that 
should be used.  That allows the tracer to easily decide whether it should 
trace the object with a simple check

Re: [v8-dev] Is Multiple 'EmbedderHeapTracer's Feasible?

[Michael Lippautz]

On Tue, Jun 12, 2018 at 8:02 AM d.haresign via v8-dev <
v8-dev@googlegroups.com> wrote:

> I work on a product which embeds Chromium.  In addition, we host complex
> C++ objects in V8 directly.  Our V8 binding layer supports the tracing of
> objects to determine references to other objects, rather than simply
> holding on to all objects with strong 'v8::Persistent's.  Up until Chromium
> 56 / V8 5.6, we were doing this tracing in the GC prologue, using
> 'Isolate::SetReference' to inform V8 of all the edges.
>
> As we look to upgrade past Chromium 59 / V8 5.9, 'Isolate::SetReference'
> has been removed in favor of the 'EmbedderHeapTracer' APIs.
>
> One issue we have with the switch, is that Chromium already sets the
> 'EmbedderHeapTracer' to its own 'ScriptWrappableVisitor' tracer.  In order
> to work we need to come up with a way to work along side Chromium.
>
> To quickly recap on how the tracing works, when an embedder hosts an
> object it can set internal fields.  When V8 is tracing objects, if it
> discovers an object which has the first and second internal fields both
> set, it considers it as a wrapper that needs to be traced with the
> 'EmbedderHeapTracer'.  These internal fields let you store information
> necessary to perform the tracing.
>
> In Chromium, the first field is a pointer to a 'WrapperTypeInfo' object,
> and the second field is a pointer to a 'ScriptWrappable' object.  The
> 'RegisterV8References' implementation in Chromium's tracer expects to only
> ever be called with these Chromium objects.  As such, it does a
> reinterpret_cast to a 'WrapperTypeInfo *' on the first field.  If the
> 'gin_embedder' field of that object is 'kEmbedderBlink', it does a
> reinterpret_cast to a 'ScriptWrappable *', and traces that object.
>
> If we were to integrate with Chromium's 'EmbedderHeapTracer', we would
> have to set up our objects to have objects which match the layout of
> Chromium's 'WrapperTypeInfo', pretend that the are 'kEmbedderBlink'
> objects, and somehow get it to call our trace methods instead of Chromiums
> (the exact way this is done seems to vary wildly over the latest 10 or so
> versions of Chromium).
>
> This seems like a rabbit hole full of issues I'd rather avoid.
>

Just to acknowledge: All of that is correct.

Now, you could still do something differently:
- Create a generic dispatcher class inheriting from EmbedderHeapTracer.
- Instead of setting ScriptWrappableVisitor, pass on the generic dispatcher
to V8.
- Have your objects match this code

that decides whether something should be passed on or not to the
EmbedderHeapTracer. In the case that these objects are not critical wrt. to
memory consumption, tge simplest thing would be to have 3 fields of which
the first 2 are nullptr.
- Have your generic dispatcher use Chromium's ScriptWrappable visitor if
it's a Blink object
- ... or dispatch to whatever EmbedderHeapTracer, otherwise.

The fragile part is the matching algorithm in V8 which we wanted to change
but ultimately never needed to. You don't have to deal with WrapperTypeInfo
or any of the types specified there as the generic visitor would just pass
on the object.

fyi: We were thinking of adding a bit somewhere on V8's hidden class that
is set through instantiating it through the template info. That bit
could've been used to signal that the object should be sent to the
EmbedderHeapTracer instead of doing the pattern matching on fields.


>
> Another option I see is to change V8 to support multiple
> 'EmbedderHeapTracer's.  Each 'EmbedderHeapTracer' would be informed of all
> the wrappers that V8 found, and each one would be responsible for tracing
> its own objects.  From testing with Chromium 62 / V8 6.2, this seems to
> work, even if there are references between objects from the two embedders.
> Consider an object graph as follows:
>
>   (root) -> (E1 O1) -> (E1 O2) -> (E2 O1) -> (E2 O2) -> (E1 O3)
>
> Where E1/E2 represents the different embedders, and O1/O2/O3 represents
> different objects.
>
> V8 would discover the wrapper (E1 O1) and inform the
> 'EmbedderHeapTracer's.  E1's tracer would trace (E1 O1), and (E2 O2).  E1's
> tracer doesn't know how to trace (E2 O1), but it informs V8 that it's
> alive.  V8 then informs the tracers about (E2 O1).  E2's tracer traces (E2
> O1), and (E2 O2).  Again, E2's tracer doesn't know how to trace (E1 O3),
> but it informs V8 that it's alive.  V8 then informs the tracers about (E1
> O3).  E1's tracer traces it, and the tracing is complete.
>
> There are a few more details which I am missing out relating to changes in
> Chromium which would be required to get it to ignore objects, but those
> should be fairly straight forward.
>
> Is there anything I am overlooking which would prevent multiple
> 'EmbedderHeapTracer's from being a viable option?
>

Pretty exciting to see such a thing work. And, yes, I would say that this
is definitely via