Bug 110025 – Element: Avoid unrelated attribute synchronization on other attribute access.

• Component: DOM
• Status: RESOLVED FIXED

[Andreas Kling]

Element: Avoid unrelated attribute synchronization on other attribute access.

[Andreas Kling]

Created attachment 188725 [details]
Patch

[Darin Adler]

Comment on attachment 188725 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=188725&action=review

I like the basic idea here, although I’m not sure that all the changes here are improvements.

> Source/WebCore/dom/Element.cpp:351
> +void Element::synchronizeAttributeIfNeeded(const QualifiedName& name) const

I don’t think “if needed” is needed in this name.

> Source/WebCore/dom/Element.cpp:367
> +void Element::synchronizeAttributeIfNeeded(const AtomicString& localName) const

The whole point of this function is to do the synchronizeAttribute operation without creating a QualifiedName. I think it needs a comment to explain that.

Do we want part of this inlined for getAttribute?

> Source/WebCore/dom/Element.cpp:769
> +    const AtomicString& caseAdjustedLocalName = shouldIgnoreAttributeCase(this) ? localName.lower() : localName;

This does a lot of unneeded work when the name is already all lowercase.

I looked at AtomicString::lower and it’s super-funny! It has a comment saying that it’s a hot function, but does not have either of the two obvious optimizations:

1) Fast case for when the string is already all lower case.
2) Avoid allocating and deleting an extra StringImpl in the common case where the lower case version of the string is already in the atomic string table.

> Source/WebCore/dom/Element.cpp:1853
> +    if (const Attribute* attribute = elementData()->getAttributeItem(localName, shouldIgnoreAttributeCase(this)))
> +        return ensureAttr(attribute->name());
> +    return 0;

I like the old style with early return for the error case better than this, even though this scopes the local variable more tightly.

> Source/WebCore/dom/Element.cpp:1864
> +    if (const Attribute* attribute = elementData()->getAttributeItem(qName))
> +        return ensureAttr(attribute->name());
> +    return 0;

Same here.

[Darin Adler]

Comment on attachment 188725 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=188725&action=review

>> Source/WebCore/dom/Element.cpp:769
>> +    const AtomicString& caseAdjustedLocalName = shouldIgnoreAttributeCase(this) ? localName.lower() : localName;
> 
> This does a lot of unneeded work when the name is already all lowercase.
> 
> I looked at AtomicString::lower and it’s super-funny! It has a comment saying that it’s a hot function, but does not have either of the two obvious optimizations:
> 
> 1) Fast case for when the string is already all lower case.
> 2) Avoid allocating and deleting an extra StringImpl in the common case where the lower case version of the string is already in the atomic string table.

Oops, I am wrong, it does have optimization (1), just not optimization (2). Sorry, my mistake.

[Andreas Kling]

(In reply to comment #2)
> (From update of attachment 188725 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188725&action=review
> 
> I like the basic idea here, although I’m not sure that all the changes here are improvements.
> 
> > Source/WebCore/dom/Element.cpp:351
> > +void Element::synchronizeAttributeIfNeeded(const QualifiedName& name) const
> 
> I don’t think “if needed” is needed in this name.

Sure, I'll drop it.

> > Source/WebCore/dom/Element.cpp:367
> > +void Element::synchronizeAttributeIfNeeded(const AtomicString& localName) const
> 
> The whole point of this function is to do the synchronizeAttribute operation without creating a QualifiedName. I think it needs a comment to explain that.

Will add.

> Do we want part of this inlined for getAttribute?

Sounds like the right thing to do, even though fastGetAttribute() is ideally already used wherever attribute retrieval speed really matters. Regardless, I'll mark these methods inline.

> I like the old style with early return for the error case better than this, even though this scopes the local variable more tightly.

Yeah okay. I was on the fence.

[Andreas Kling]

Committed r143112: <http://trac.webkit.org/changeset/143112>

[Csaba Osztrogonác]

(In reply to comment #5)
> Committed r143112: <http://trac.webkit.org/changeset/143112>

... and a buildfix landed in https://trac.webkit.org/changeset/143127

( But unfortunately I can't ensure if it builds everywhere, 
because build.webkit.org is out of order now. :-/ )