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.
Created attachment 261867 [details] Patch
(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.
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.
(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.
Created attachment 261916 [details] Patch for landing
Created attachment 261919 [details] Rebasing for landing
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
Created attachment 261920 [details] Rebasing for landing
Comment on attachment 261920 [details] Rebasing for landing Clearing flags on attachment: 261920 Committed r190239: <http://trac.webkit.org/changeset/190239>
All reviewed patches have been landed. Closing bug.
(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.
(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.
Filed bug 149556 to keep track of this
(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!
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