Avoid a virtual function call in HTMLFormControlElement::isDisabledOrReadOnly()
Created attachment 419105 [details] Patch
Comment on attachment 419105 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=419105&action=review > Source/WebCore/ChangeLog:9 > + Avoid calling isDisabledFormControl() which is a virtual function on Element. > + HTMLFormControlElement is the only class in its hierarchy that implements this, I don't think this is right. HTMLOptGroupElement, HTMLOptionElement, and SliderThumbElement overrides this.
Comment on attachment 419105 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=419105&action=review >> Source/WebCore/ChangeLog:9 >> + HTMLFormControlElement is the only class in its hierarchy that implements this, > > I don't think this is right. HTMLOptGroupElement, HTMLOptionElement, and SliderThumbElement overrides this. Nvm.
Committed r272299: <https://trac.webkit.org/changeset/272299> All reviewed patches have been landed. Closing bug and clearing flags on attachment 419105 [details].
<rdar://problem/73917813>
Comment on attachment 419105 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=419105&action=review > Source/WebCore/html/HTMLFormControlElement.h:116 > + bool isDisabledOrReadOnly() const { return m_disabled || m_disabledByAncestorFieldset || m_isReadOnly; } Can this optimization be done by adding final somewhere instead?
Comment on attachment 419105 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=419105&action=review >> Source/WebCore/html/HTMLFormControlElement.h:116 >> + bool isDisabledOrReadOnly() const { return m_disabled || m_disabledByAncestorFieldset || m_isReadOnly; } > > Can this optimization be done by adding final somewhere instead? I don’t get it — given that classes override isDisabledFormControl, is this change correct?
(In reply to Darin Adler from comment #7) > Comment on attachment 419105 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=419105&action=review > > >> Source/WebCore/html/HTMLFormControlElement.h:116 > >> + bool isDisabledOrReadOnly() const { return m_disabled || m_disabledByAncestorFieldset || m_isReadOnly; } > > > > Can this optimization be done by adding final somewhere instead? > > I don’t get it — given that classes override isDisabledFormControl, is this > change correct? None of the classes that override isDisabledFormControl() are HTMLFormControlElement subclasses. Would marking HTMLFormControlElement::isDisabledFormControl() final be equivalent? I'm not sure what compiler optimizations are enabled by 'final'.
(In reply to Simon Fraser (smfr) from comment #8) > (In reply to Darin Adler from comment #7) > > Comment on attachment 419105 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=419105&action=review > > > > >> Source/WebCore/html/HTMLFormControlElement.h:116 > > >> + bool isDisabledOrReadOnly() const { return m_disabled || m_disabledByAncestorFieldset || m_isReadOnly; } > > > > > > Can this optimization be done by adding final somewhere instead? > > > > I don’t get it — given that classes override isDisabledFormControl, is this > > change correct? > > None of the classes that override isDisabledFormControl() are > HTMLFormControlElement subclasses. > > Would marking HTMLFormControlElement::isDisabledFormControl() final be > equivalent? I'm not sure what compiler optimizations are enabled by 'final'. It'd avoid stable pointer lookup!
(In reply to Simon Fraser (smfr) from comment #8) > Would marking HTMLFormControlElement::isDisabledFormControl() final be > equivalent? I'm not sure what compiler optimizations are enabled by 'final'. Yes, I think it would be equivalent, as long as the implementation of isDisabledFormControl was in the header. The combination of final and inlining should do exactly the same as this change. And the bonus is that it would optimize any other similar call sites in the same way.
(In reply to Ryosuke Niwa from comment #9) > It'd avoid stable pointer lookup! Ryosuke means "vtable pointer lookup". Basically, if we didn’t inline, then the function would be faster, and who knows how good the link time optimization is at giving the same benefits as inlining. But, as I said, with inlining it should literally be the same.
(In reply to Darin Adler from comment #11) > (In reply to Ryosuke Niwa from comment #9) > > It'd avoid stable pointer lookup! > > Ryosuke means "vtable pointer lookup". Basically, if we didn’t inline, then > the function would be faster, and who knows how good the link time > optimization is at giving the same benefits as inlining. But, as I said, > with inlining it should literally be the same. Addressing this in https://bugs.webkit.org/show_bug.cgi?id=222783