Bug 110025 - Element: Avoid unrelated attribute synchronization on other attribute access.
Summary: Element: Avoid unrelated attribute synchronization on other attribute access.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords:
Depends on:
Blocks: 109505
  Show dependency treegraph
 
Reported: 2013-02-16 13:03 PST by Andreas Kling
Modified: 2013-02-17 01:31 PST (History)
5 users (show)

See Also:


Attachments
Patch (16.77 KB, patch)
2013-02-16 13:03 PST, Andreas Kling
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2013-02-16 13:03:05 PST
Element: Avoid unrelated attribute synchronization on other attribute access.
Comment 1 Andreas Kling 2013-02-16 13:03:49 PST
Created attachment 188725 [details]
Patch
Comment 2 Darin Adler 2013-02-16 13:21:52 PST
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.
Comment 3 Darin Adler 2013-02-16 13:22:37 PST
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.
Comment 4 Andreas Kling 2013-02-16 13:42:01 PST
(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.
Comment 5 Andreas Kling 2013-02-16 14:58:54 PST
Committed r143112: <http://trac.webkit.org/changeset/143112>
Comment 6 Csaba Osztrogonác 2013-02-17 01:31:47 PST
(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. :-/ )