Bug 228541 – ListAttributeTargetObserver is needlessly created even when there is no list attribute

• Component: New Bugs
• Status: RESOLVED FIXED

[Maciej Stachowiak]

ListAttributeTargetObserver is needlessly created even when there is no list attribute

[Maciej Stachowiak]

Created attachment 434410 [details]
Patch

[Maciej Stachowiak]

Patch is for EWS only.

[Darin Adler]

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?

[Maciej Stachowiak]

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?

[Darin Adler]

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.

[Darin Adler]

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.

[Yusuke Suzuki]

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);
}

[Maciej Stachowiak]

Created attachment 434706 [details]
Patch

[Maciej Stachowiak]

Created attachment 434707 [details]
Patch

[Maciej Stachowiak]

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.

[Yusuke Suzuki]

Comment on attachment 434707 [details]
Patch

r=me

[EWS]

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

[Radar WebKit Bug Importer]

<rdar://problem/81370699>