221319 2021-02-02 21:51:31 -0800 Avoid a virtual function call in HTMLFormControlElement::isDisabledOrReadOnly() 2021-03-05 00:44:18 -0800 1 1 1 Unclassified WebKit Forms WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=222783 InRadar P2 Normal --- 1 simon.fraser simon.fraser cdumez changseok darin esprehn+autocc ews-watchlist gyuyoung.kim mifenton rniwa simon.fraser webkit-bug-importer wenson_hsieh oldest_to_newest 1725017 0 simon.fraser 2021-02-02 21:51:31 -0800 Avoid a virtual function call in HTMLFormControlElement::isDisabledOrReadOnly() 1725019 1 419105 simon.fraser 2021-02-02 21:53:39 -0800 Created attachment 419105 Patch 1725022 2 419105 rniwa 2021-02-02 22:06:34 -0800 Comment on attachment 419105 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. 1725025 3 419105 rniwa 2021-02-02 22:09:43 -0800 Comment on attachment 419105 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. 1725045 4 ews-feeder 2021-02-02 23:14:30 -0800 Committed r272299: <https://trac.webkit.org/changeset/272299> All reviewed patches have been landed. Closing bug and clearing flags on attachment 419105. 1725046 5 webkit-bug-importer 2021-02-02 23:15:19 -0800 <rdar://problem/73917813> 1725427 6 419105 darin 2021-02-03 18:16:53 -0800 Comment on attachment 419105 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? 1725429 7 419105 darin 2021-02-03 18:19:38 -0800 Comment on attachment 419105 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? 1725447 8 simon.fraser 2021-02-03 19:21:16 -0800 (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'. 1725488 9 rniwa 2021-02-03 21:37:07 -0800 (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! 1725679 10 darin 2021-02-04 10:03:58 -0800 (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. 1725680 11 darin 2021-02-04 10:05:13 -0800 (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. 1736208 12 rniwa 2021-03-05 00:44:18 -0800 (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 419105 2021-02-02 21:53:39 -0800 2021-02-02 23:14:32 -0800 Patch bug-221319-20210202215339.patch text/plain 1763 simon.fraser U3VidmVyc2lvbiBSZXZpc2lvbjogMjcyMjE4CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNTRiOWUxYzFjNmEzZmVl Y2YyNTJlZTJlNWQ5MGJmN2M4MDY0N2U4MC4uYTZlN2NlZDE4ZjU2NmIwNWFiMzQwNGIyOTdkZjM2 ZmRhYjUyZTIxMSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE3IEBACisyMDIxLTAyLTAyICBTaW1v biBGcmFzZXIgIDxzaW1vbi5mcmFzZXJAYXBwbGUuY29tPgorCisgICAgICAgIEF2b2lkIGEgdmly dHVhbCBmdW5jdGlvbiBjYWxsIGluIEhUTUxGb3JtQ29udHJvbEVsZW1lbnQ6OmlzRGlzYWJsZWRP clJlYWRPbmx5KCkKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dp P2lkPTIyMTMxOQorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAg ICAgIEF2b2lkIGNhbGxpbmcgaXNEaXNhYmxlZEZvcm1Db250cm9sKCkgd2hpY2ggaXMgYSB2aXJ0 dWFsIGZ1bmN0aW9uIG9uIEVsZW1lbnQuCisgICAgICAgIEhUTUxGb3JtQ29udHJvbEVsZW1lbnQg aXMgdGhlIG9ubHkgY2xhc3MgaW4gaXRzIGhpZXJhcmNoeSB0aGF0IGltcGxlbWVudHMgdGhpcywK KyAgICAgICAgc28gd2UgY2FuIGp1c3QgY29weSBpdHMgaW1wbGVtZW50YXRpb24uCisKKyAgICAg ICAgKiBodG1sL0hUTUxGb3JtQ29udHJvbEVsZW1lbnQuaDoKKyAgICAgICAgKFdlYkNvcmU6OkhU TUxGb3JtQ29udHJvbEVsZW1lbnQ6OmlzRGlzYWJsZWRPclJlYWRPbmx5IGNvbnN0KToKKwogMjAy MS0wMi0wMiAgU2ltb24gRnJhc2VyICA8c2ltb24uZnJhc2VyQGFwcGxlLmNvbT4KIAogICAgICAg ICBSZW5hbWUgSFRNTEZvcm1Db250cm9sRWxlbWVudDo6c2V0TmVlZHNXaWxsVmFsaWRhdGVDaGVj aygpIHRvIHVwZGF0ZVdpbGxWYWxpZGF0ZUFuZFZhbGlkaXR5KCkKZGlmZiAtLWdpdCBhL1NvdXJj ZS9XZWJDb3JlL2h0bWwvSFRNTEZvcm1Db250cm9sRWxlbWVudC5oIGIvU291cmNlL1dlYkNvcmUv aHRtbC9IVE1MRm9ybUNvbnRyb2xFbGVtZW50LmgKaW5kZXggMzk4ZGYwYTkwZjNjZDZlNmMzYTQ0 MWM4YTE4NTI4YjY3Y2Q4NzBhOC4uNzc5Y2ZjYjJiZDIzMjZkNTNmNWM3YmNlMzg0MDY5MDk2NmQ4 Y2U1OSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvaHRtbC9IVE1MRm9ybUNvbnRyb2xFbGVt ZW50LmgKKysrIGIvU291cmNlL1dlYkNvcmUvaHRtbC9IVE1MRm9ybUNvbnRyb2xFbGVtZW50LmgK QEAgLTExMyw3ICsxMTMsNyBAQCBwdWJsaWM6CiAgICAgdm9pZCBzZXRDdXN0b21WYWxpZGl0eShj b25zdCBTdHJpbmcmKSBvdmVycmlkZTsKIAogICAgIGJvb2wgaXNSZWFkT25seSgpIGNvbnN0IHsg cmV0dXJuIG1faXNSZWFkT25seTsgfQotICAgIGJvb2wgaXNEaXNhYmxlZE9yUmVhZE9ubHkoKSBj b25zdCB7IHJldHVybiBpc0Rpc2FibGVkRm9ybUNvbnRyb2woKSB8fCBtX2lzUmVhZE9ubHk7IH0K KyAgICBib29sIGlzRGlzYWJsZWRPclJlYWRPbmx5KCkgY29uc3QgeyByZXR1cm4gbV9kaXNhYmxl ZCB8fCBtX2Rpc2FibGVkQnlBbmNlc3RvckZpZWxkc2V0IHx8IG1faXNSZWFkT25seTsgfQogCiAg ICAgYm9vbCBoYXNBdXRvZm9jdXNlZCgpIHsgcmV0dXJuIG1faGFzQXV0b2ZvY3VzZWQ7IH0KICAg ICB2b2lkIHNldEF1dG9mb2N1c2VkKCkgeyBtX2hhc0F1dG9mb2N1c2VkID0gdHJ1ZTsgfQo=