Bug 221319 – Avoid a virtual function call in HTMLFormControlElement::isDisabledOrReadOnly()

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Simon Fraser (smfr)]

Avoid a virtual function call in HTMLFormControlElement::isDisabledOrReadOnly()

[Simon Fraser (smfr)]

Created attachment 419105 [details]
Patch

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

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.

[EWS]

Committed r272299: <https://trac.webkit.org/changeset/272299>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 419105 [details].

[Radar WebKit Bug Importer]

<rdar://problem/73917813>

[Darin Adler]

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?

[Darin Adler]

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?

[Simon Fraser (smfr)]

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

[Ryosuke Niwa]

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

[Darin Adler]

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

[Darin Adler]

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

[Ryosuke Niwa]

(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