Bug 149522 – Remove the need for DOMClass in case of JSBuiltinConstructor WebIDL

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

When a WebIDL interface is defined as JSBuiltinConstructor, the DOM class is really a dummy class.
While it may be good to remove the need to allocate a DOM class object alltogether, an intermediate approach might just be to generate the dummy class by the binding generator.

[youenn fablet]

Created attachment 261867 [details]
Patch

[youenn fablet]

(In reply to comment #1)
> Created attachment 261867 [details]
> Patch

Took the simple approach of generating the dummy DOM class.
It might be nice to remove the need for the memory allocation and counter at a later stage.

[Darin Adler]

Comment on attachment 261867 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=261867&action=review

This is OK and an improvement, but I think we can easily go a bit further.

> Source/WebCore/bindings/js/JSDOMWrapper.h:52
> +class DummyDOMClass: public RefCounted<DummyDOMClass> {

Missing space before ":".

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

> Source/WebCore/bindings/js/JSDOMWrapper.h:56
> +protected:
> +    DummyDOMClass() { }

This doesn’t really achieve all that much. I think we’d want the constructor to be private in the actual generated classes too, not just this dummy class. I’d be slightly tempted to just leave this out entirely and let the constructor be public.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:5099
> +    push(@headerContent, "    class $className : public DummyDOMClass {\n");

If all my comments about DummyDOMClass a correct, then I think we might just be able to derive from RefCounted<$className> here and not have DummyDOMClass at all.

[youenn fablet]

(In reply to comment #3)
> Comment on attachment 261867 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=261867&action=review
> 
> This is OK and an improvement, but I think we can easily go a bit further.
> 
> > Source/WebCore/bindings/js/JSDOMWrapper.h:52
> > +class DummyDOMClass: public RefCounted<DummyDOMClass> {
> 
> Missing space before ":".
> 
> > 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.

> 
> > Source/WebCore/bindings/js/JSDOMWrapper.h:56
> > +protected:
> > +    DummyDOMClass() { }
> 
> This doesn’t really achieve all that much. I think we’d want the constructor
> to be private in the actual generated classes too, not just this dummy
> class. I’d be slightly tempted to just leave this out entirely and let the
> constructor be public.
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:5099
> > +    push(@headerContent, "    class $className : public DummyDOMClass {\n");
> 
> If all my comments about DummyDOMClass a correct, then I think we might just
> be able to derive from RefCounted<$className> here and not have
> DummyDOMClass at all.

OK. Let's remove DummyDOMClass and generate the destructor directly then.

[youenn fablet]

Created attachment 261916 [details]
Patch for landing

[youenn fablet]

Created attachment 261919 [details]
Rebasing for landing

[WebKit Commit Bot]

Comment on attachment 261919 [details]
Rebasing for landing

Rejecting attachment 261919 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 261919, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/210930

[youenn fablet]

Created attachment 261920 [details]
Rebasing for landing

[WebKit Commit Bot]

Comment on attachment 261920 [details]
Rebasing for landing

Clearing flags on attachment: 261920

Committed r190239: <http://trac.webkit.org/changeset/190239>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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

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.

[youenn fablet]

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

[youenn fablet]

Filed bug 149556 to keep track of this

[Darin Adler]

(In reply to comment #12)
> 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!

[Chris Dumez]

Comment on attachment 261920 [details]
Rebasing for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=261920&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1751
> +    AddIncludesForTypeInImpl($implType) if not UseDummyDOMClass($interface);

if not == unless