Bug 102240 – [V8] Rename dispatchWrap

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dan Carney]

[V8] Rename dispatchWrap

[Dan Carney]

Created attachment 174172 [details]
Patch

[Kentaro Hara]

Good refactoring.

[WebKit Review Bot]

Comment on attachment 174172 [details]
Patch

Clearing flags on attachment: 174172

Committed r134696: <http://trac.webkit.org/changeset/134696>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kentaro Hara]

Forgive me, it looks like I am behind your wrap/createWrapper/toV8/toV8Fast work...

(1) What is [V8NoWrapperCache] for ? I am expecting that [CustomToJSObject] and [V8CustomToJSObject] would be sufficient.

(2) Why do you need to let developers write both custom toV8() and custom wrap() ? Previously we supported custom toV8() only and didn't support custom wrap(). Why do you now need to support both custom toV8() and custom wrap() ?

[Dan Carney]

(In reply to comment #5)
> Forgive me, it looks like I am behind your wrap/createWrapper/toV8/toV8Fast work...
> 
> (1) What is [V8NoWrapperCache] for ? I am expecting that [CustomToJSObject] and [V8CustomToJSObject] would be sufficient.
> 
> (2) Why do you need to let developers write both custom toV8() and custom wrap() ? Previously we supported custom toV8() only and didn't support custom wrap(). Why do you now need to support both custom toV8() and custom wrap() ?

I moved the functionality around.  wrap used to first check the wrapper cache then built the wrapped object. toV8 checked the wrapper, then called wrap, which often checked the cache a number of times. I ensured now that the check is only performed once, in toV8 (or toV8fast, or toV8Object), and then the call is passed to wrap. No wrap call should be made outside of the bindings.

A V8NoWrapperCache means that there will never be a cache used for such object, so wrap is not generated, and in that case, we must have a custom v8 implementation since we have no idea what the intention is such a call.

Sometimes a wrap implementation  might want to use the wrapper cache though, and just do some type checking and dispatch the wrap call down the subclass chain. That's marked as V8CustomToJSObject or CustomToJSObject.

By splitting functionality I ensured that the wrapper cache check is pretty much always autogenerated and performed just once. Additionally, it meant that toV8 and toV8Fast can almost always be generated, which wasn't previously the case for classes marked (V8)CustomToJSObject

[Kentaro Hara]

(In reply to comment #6)
> I moved the functionality around.  wrap used to first check the wrapper cache then built the wrapped object. toV8 checked the wrapper, then called wrap, which often checked the cache a number of times. I ensured now that the check is only performed once, in toV8 (or toV8fast, or toV8Object), and then the call is passed to wrap. No wrap call should be made outside of the bindings.
> 
> A V8NoWrapperCache means that there will never be a cache used for such object, so wrap is not generated, and in that case, we must have a custom v8 implementation since we have no idea what the intention is such a call.
> 
> Sometimes a wrap implementation  might want to use the wrapper cache though, and just do some type checking and dispatch the wrap call down the subclass chain. That's marked as V8CustomToJSObject or CustomToJSObject.
> 
> By splitting functionality I ensured that the wrapper cache check is pretty much always autogenerated and performed just once. Additionally, it meant that toV8 and toV8Fast can almost always be generated, which wasn't previously the case for classes marked (V8)CustomToJSObject

Thanks, I understand that we now have only one cache check.

[1] However, from the perspective of developers, the current IDL attributes look a bit confusing.

As far as I see:

- If we want to write custom toV8(), we need to specify [(V8)CustomToJSObject] and [V8NoWrapperCache].

- If we want to write custom wrap(), we need to specify [(V8)CustomToJSObject] only.

- Then what should we specify if we want to write both custom toV8() and custom wrap() ?

I'd be happy if we could clean up the IDL attributes.


[2] Do we really need to expose both custom toV8() and custom wrap() to make the number of cache checks once ?


[3] Just to clarify, would you elaborate on why the following structure won't work? (I'd like to understand what is making the situation complicated.)

  call path: toV8Fast() => toV8() => createWrapper()

  - toV8Fast() does the cache check.
  - toV8() can be custom to let developers write any delegation logic.
  - createWrapper() creates a new V8 object.

[Dan Carney]

> - If we want to write custom toV8(), we need to specify [(V8)CustomToJSObject] and [V8NoWrapperCache].

no, you just need V8NoWrapperCache, which means we don't care about caching, which in turn means we can't generate a toV8, so you must provide one.

> - Then what should we specify if we want to write both custom toV8() and custom wrap() ?

it's not a case that makes sense. either we want the wrapper cache, in which case toV8 is automatically generated, or we don't, in which case we must supply toV8 ourselves, and all wrap methods will be not generated.

> [2] Do we really need to expose both custom toV8() and custom wrap() to make the number of cache checks once ?

yes, because a custom toV8 function has no wrapper checks.

> [3] Just to clarify, would you elaborate on why the following structure won't work? (I'd like to understand what is making the situation complicated.)
> 
>   call path: toV8Fast() => toV8() => createWrapper()
> 
>   - toV8Fast() does the cache check.
>   - toV8() can be custom to let developers write any delegation logic.
>   - createWrapper() creates a new V8 object.

that's essentially what we have, except if you rename toV8Fast to toV8 and toV8 to wrap. 


the problem with what you seem to want is that toV8Fast is only suitable for some places, so toV8 also needs to exist and do a wrapper check, which means 2 wrapper checks for those places

[Kentaro Hara]

Thank you very much for the clarification.

> > - If we want to write custom toV8(), we need to specify [(V8)CustomToJSObject] and [V8NoWrapperCache].
> 
> no, you just need V8NoWrapperCache, which means we don't care about caching, which in turn means we can't generate a toV8, so you must provide one.

Got it. I understood the situation, but I guess that the IDL attributes are confusing. In JSC, [(JS)CustomToJSObject] indicates that we can write custom toJS(). In V8, [(V8)CustomToJSObject] indicates that we can write custom wrap(). [V8NoWrapperCache] indicates that we can write custom toV8().

Maybe we want to change it like this?

- [(V8)CustomToJSObject] => we can write custom toV8()
- [V8CustomWrap] => we can write custom wrap()


Another confusion comes from the fact that [(V8)CustomToJSObject] is specified for all IDL files that have [V8NoWrapperCache]. The [(V8)CustomToJSObject] is useless (ignored in code generators). We can remove the [(V8)CustomToJSObject] from those IDL files.



> > [3] Just to clarify, would you elaborate on why the following structure won't work? (I'd like to understand what is making the situation complicated.)
> > 
> >   call path: toV8Fast() => toV8() => createWrapper()
> > 
> >   - toV8Fast() does the cache check.
> >   - toV8() can be custom to let developers write any delegation logic.
> >   - createWrapper() creates a new V8 object.
> 
> that's essentially what we have, except if you rename toV8Fast to toV8 and toV8 to wrap. 
> 
> the problem with what you seem to want is that toV8Fast is only suitable for some places, so toV8 also needs to exist and do a wrapper check, which means 2 wrapper checks for those places

Thanks, I understood the situation. (I'm thinking about how we can make it simpler. My colleague asked me to simplify it because the current situation is complicated:-) toV8Fast(), toV8(), toV8Object(), wrap(), createWrapper()...)

[Dan Carney]

> Maybe we want to change it like this?
> 
> - [(V8)CustomToJSObject] => we can write custom toV8()
> - [V8CustomWrap] => we can write custom wrap()

CustomToJSObject can't be renamed since jsc uses it.  V8 bindings would have to stop using it almost altogether. You'd have to split every current instance of it into JSCustomToJSObject and V8CustomWrap and then if you renamed V8NoWrapperCache to V8CustomToJSObject, you could recollect the cases where V8CustomToJSObject and JSCustomToJSObject coincided into CustomToJSObject, which wouldn't be many places.

> Another confusion comes from the fact that [(V8)CustomToJSObject] is specified for all IDL files that have [V8NoWrapperCache]. The [(V8)CustomToJSObject] is useless (ignored in code generators). We can remove the [(V8)CustomToJSObject] from those IDL files.

The redundancy doesn't really matter. I think I may even have added an assertion that one implied the other.

Anyway, if you want to rename stuff, I certainly don't mind either way, and it would definitely be a small improvement in the readability of the code gen file.

[Dan Carney]

> Thanks, I understood the situation. (I'm thinking about how we can make it simpler. My colleague asked me to simplify it because the current situation is complicated:-) toV8Fast(), toV8(), toV8Object(), wrap(), createWrapper()...)

unfortunately, it's important to generate all those different functions for every class  to have fast dispatch down the subclass chains and to have fast wrapper cache checks. there will be quite a few more toFastV8 function being added soon to every class that supports toV8