Bug 149556 – Improve binding of JSBuiltinConstructor classes

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

Following on bug 149522 discussion, we might want to improve the binding generator.

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #4)
> > > (In reply to comment #3)
> > > > > Source/WebCore/bindings/js/JSDOMWrapper.h:54
> > > > > +    virtual ~DummyDOMClass() { }
> > > > 
> > > > Not sure why this is needed. Do we need the class to seem polymorphic for
> > > > some reason?
> > > 
> > > This is required by RefCounted IIRC.
> > 
> > RefCounted itself definitely does *not* require that the class be
> > polymorphic so it doesn’t require a virtual destructor.
> > 
> > We need to make the destructor virtual if we are going to deref or delete
> > these objects in a polymorphic way, using a pointer to a base class. I think
> > you could revisit this and make the class both "final" and not define a
> > virtual destructor.
> 
> Thanks for the information.
> 
> > 
> > You also talked about cleaning things up further so the class isn’t needed
> > at all; if you do that this won’t matter at all.
> > 
> > If we do need to keep this class, then we can do another trick for better
> > efficiency:
> > 
> >     class $className {
> >     public:
> >         static Ref<$className> create() { return adoptRef(singleton());
> > }\n"); 
> >         static void ref() { }
> >         static void deref() { }
> >     private:
> >         static $className& singleton() { static NeverDestroyed<$className>
> > singleton; return singleton; }
> >     };
> > 
> > No memory allocation, no incrementing and decrementing reference counts.
> 
> Yes, that is one approach I thought of.
> It requires bypassing JSDOMGlobalObject wrapper cache, which is another
> efficiency bonus.
> 
> We may also want to use a single singleton for all builtin classes.
> 
> To go further, we may want to skip generation of some needless code (toJS,
> releaseImpl...).
> 
> Choice may be guided by the added complexity to the binding generator.
> I plan to take some time on this once the JS builtin architecture is ready
> to implement ReadableStream and WritableStream.

[Darin Adler]

> It requires bypassing JSDOMGlobalObject wrapper cache, which is another
> efficiency bonus.

My proposal above does not require bypassing the wrapper cache. The singleton wrapper will simply be found in the cache (many times over).

However, of course we can do further optimization to bypass the wrapper cache.

> We may also want to use a single singleton for all builtin classes.

We may not need an object at all if we are changing things like the wrapper class. I think we should create a JavaScript object that doesn’t wrap a DOM object at all. And I’m not even sure it’s all that challenging to do that. I might take a crack at it.

> To go further, we may want to skip generation of some needless code (toJS,
> releaseImpl...).
> 
> Choice may be guided by the added complexity to the binding generator.
> I plan to take some time on this once the JS builtin architecture is ready
> to implement ReadableStream and WritableStream.

You may be misunderstanding.

My specific proposed change above is entirely local and doesn’t add any complexity to the bindings generator.

Your more ambitious project is different; that will require a change to the bindings generator, but even that probably does not require not a huge one!

[youenn fablet]

(In reply to comment #1)
> > It requires bypassing JSDOMGlobalObject wrapper cache, which is another
> > efficiency bonus.
> 
> My proposal above does not require bypassing the wrapper cache. The
> singleton wrapper will simply be found in the cache (many times over).

I may be missing something here.
My understanding is that, if using a singleton, toJS(XX::create()) will always return the same JSXX object. This is not what we want, we want a new object each time. 
Hence why I think we should bypass the cache and remove the generation of the toJS(XX) function. Once done, the singleton class might be made private to the JS class or applicable to all classes.

> We may not need an object at all if we are changing things like the wrapper
> class. I think we should create a JavaScript object that doesn’t wrap a DOM
> object at all. And I’m not even sure it’s all that challenging to do that. I
> might take a crack at it.

I agree, that is the idea behind removing toJS, releaseImpl and so on...
I haven't done the exercise so cannot comment on how much complexity it will add to the binding generator.

You are probably right about the limited complexity it will need.
But the binding generator seems already a bit messy to me.

Do not hesitate to move that forward directly.

[Darin Adler]

(In reply to comment #2)
> This is not what we want, we want a new object each time.

Why?

[youenn fablet]

(In reply to comment #3)
> (In reply to comment #2)
> > This is not what we want, we want a new object each time.
> 
> Why?

All other wrapper classes have that behavior.
I think (jsObject == toJS(toWrapped(jsObject))) is true for all classes for instance.
Breaking that behavior might not break any existing code but this looks misleading to me.

Removing generation of that code (toJS, toWrapped, Wrapper owner code) for JS builtin constructed classes makes sense to me, whether we keep a dummy DOM class or not.

With limited refactoring, this might be healthy for the binding generator code as well.

[Darin Adler]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > This is not what we want, we want a new object each time.
> > 
> > Why?
> 
> All other wrapper classes have that behavior.
> I think (jsObject == toJS(toWrapped(jsObject))) is true for all classes for
> instance.
> Breaking that behavior might not break any existing code but this looks
> misleading to me.

Sure, slightly inelegant, but I’m almost certain it will work just fine.

> Removing generation of that code (toJS, toWrapped, Wrapper owner code) for
> JS builtin constructed classes makes sense to me, whether we keep a dummy
> DOM class or not.
> 
> With limited refactoring, this might be healthy for the binding generator
> code as well.

Sure.

[youenn fablet]

Created attachment 262056 [details]
Patch

[WebKit Commit Bot]

Attachment 262056 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/scripts/test/ObjC/DOMTestJSBuiltinConstructor.mm:30:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

(In reply to comment #6)
> Created attachment 262056 [details]
> Patch

The handling of m_impl and related methods is not perfect.
It might be better to templatize it outside of the binding generator.
Maybe as a follow-up patch.

[WebKit Commit Bot]

Comment on attachment 262056 [details]
Patch

Clearing flags on attachment: 262056

Committed r190314: <http://trac.webkit.org/changeset/190314>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.