ListAttributeTargetObserver is needlessly created even when there is no list attribute
Created attachment 434410 [details] Patch
Patch is for EWS only.
Comment on attachment 434410 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434410&action=review > Source/WebCore/html/HTMLInputElement.cpp:1686 > + const AtomString& listAttrValue = attributeWithoutSynchronization(listAttr); > + if (!listAttrValue.isNull() && isConnected()) Could put this all inside the if and use auto: if (auto& value = attributeWithoutSynchronization(listAttr); !value.isNull() && isConnected()) But also, if the element is not connected, why fetch the attribute at all?
Comment on attachment 434410 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434410&action=review >> Source/WebCore/html/HTMLInputElement.cpp:1686 >> + if (!listAttrValue.isNull() && isConnected()) > > Could put this all inside the if and use auto: > > if (auto& value = attributeWithoutSynchronization(listAttr); !value.isNull() && isConnected()) > > But also, if the element is not connected, why fetch the attribute at all? If that's the currently more preferred style, I can make those first two changes. Checkin isConnected seems like it could be faster than an attribute fetch, but I think I'd have to use nested ifs to both avoid the attribute fetch for disconnected elements and avoid double hash lookup when the list attribute is actually present. Worth it?
Comment on attachment 434410 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434410&action=review >>> Source/WebCore/html/HTMLInputElement.cpp:1686 >>> + if (!listAttrValue.isNull() && isConnected()) >> >> Could put this all inside the if and use auto: >> >> if (auto& value = attributeWithoutSynchronization(listAttr); !value.isNull() && isConnected()) >> >> But also, if the element is not connected, why fetch the attribute at all? > > If that's the currently more preferred style, I can make those first two changes. > > Checkin isConnected seems like it could be faster than an attribute fetch, but I think I'd have to use nested ifs to both avoid the attribute fetch for disconnected elements and avoid double hash lookup when the list attribute is actually present. Worth it? Yes, I think we’re getting good results from scoping things using this new style. I personally think it would be worth the nested if, but it’s a close call. Not a real performance tradeoff I guess because one more hash table lookup isn’t huge given how rarely this function would be called. Sadly when the attribute itself changes, the attributeWithoutSynchronization(listAttr) call isn’t needed.
To be clear on that last point, there are really three callers here, one where the attribute value changed, and two where the connectedness changed. Could save a hash table lookup in the case where the attribute value changed, because the caller already has the attribute value. But not sure that’s worth doing for such a micro-optimization.
Comment on attachment 434410 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434410&action=review >>>> Source/WebCore/html/HTMLInputElement.cpp:1686 >>>> + if (!listAttrValue.isNull() && isConnected()) >>> >>> Could put this all inside the if and use auto: >>> >>> if (auto& value = attributeWithoutSynchronization(listAttr); !value.isNull() && isConnected()) >>> >>> But also, if the element is not connected, why fetch the attribute at all? >> >> If that's the currently more preferred style, I can make those first two changes. >> >> Checkin isConnected seems like it could be faster than an attribute fetch, but I think I'd have to use nested ifs to both avoid the attribute fetch for disconnected elements and avoid double hash lookup when the list attribute is actually present. Worth it? > > Yes, I think we’re getting good results from scoping things using this new style. > > I personally think it would be worth the nested if, but it’s a close call. Not a real performance tradeoff I guess because one more hash table lookup isn’t huge given how rarely this function would be called. Sadly when the attribute itself changes, the attributeWithoutSynchronization(listAttr) call isn’t needed. I think hashtable lookup is not free, so if we can avoid, then we should. :) In this case, isConnected() is just a bit check, so the following looks better to me. if (isConnected()) { if (const AtomString& listAttrValue = attributeWithoutSynchronization(listAttr); !listAttrValue.isNull()) m_listAttributeTargetObserver = makeUnique<ListAttributeTargetObserver>(listAttrValue, this); }
Created attachment 434706 [details] Patch
Created attachment 434707 [details] Patch
I made the changes suggested by Darin and Yusuke. I'm not sure it's more readable to have the declaration inside the if. This first version seems harder to follow: if (isConnected()) { if (auto& listAttrValue = attributeWithoutSynchronization(listAttr); !listAttrValue.isNull()) m_listAttributeTargetObserver = makeUnique<ListAttributeTargetObserver>(listAttrValue, this); } This seems easier; one extra line, but it's easier to see wha the actual if condition is. if (isConnected()) { auto& listAttrValue = attributeWithoutSynchronization(listAttr); if (!listAttrValue.isNull()) m_listAttributeTargetObserver = makeUnique<ListAttributeTargetObserver>(listAttrValue, this); } I'm happy to commit either way though. I posted the first version for review.
Comment on attachment 434707 [details] Patch r=me
Committed r280521 (240153@main): <https://commits.webkit.org/240153@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 434707 [details].
<rdar://problem/81370699>