Bug 126525 – Move function calls outside loop in dom

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Laszlo Vidacs]

length() method is called in each iteration in the for loop in dom (DOMImplementation.cpp, ElementData.cpp, EventContext.cpp), similarly to https://bugs.webkit.org/show_bug.cgi?id=125916

[Laszlo Vidacs]

Created attachment 220435 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 220435 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Do not call length() in each iteration.

How did you measure that this improved performance? Could you please post the numbers?

[Laszlo Vidacs]

No measurements performed, just omit some unnecessary method calls. I see a possible problem here only when the length is changed within the loop.

[Laszlo Vidacs]

Created attachment 223955 [details]
Patch

[Laszlo Vidacs]

I ran several DOM performance tests on efl, but from this data I cant draw conclusions about the performance of the patch. In fact, 
at 1 place of the 4 places updated in this patch, the length() method call is meantime moved outside the loop: http://trac.webkit.org/changeset/162394/trunk/Source/WebCore/dom/ElementData.cpp
So the patch is updated to cope with the remaining three places.

[Brent Fulgham]

Comment on attachment 223955 [details]
Patch

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

> Source/WebCore/dom/ElementData.cpp:146
>          m_attributeVector.uncheckedAppend(other.m_attributeArray[i]);

Honestly this would probably be better as a new C++11 loop.

[Laszlo Vidacs]

Created attachment 225058 [details]
Patch

[Laszlo Vidacs]

Thanks for the feedback, I tried the following code:
for (const Attribute& i : other.m_attributeArray)
    m_attributeVector.uncheckedAppend(i);
Unfortunately m_attributeArray is defined as a zero size array (ShareableElementData.h: Attribute m_attributeArray[0];), so the range based loop does not iterate through it.

There is a minor change in the patch to use the stored value in reserveCapacity().

[WebKit Commit Bot]

Comment on attachment 225058 [details]
Patch

Clearing flags on attachment: 225058

Committed r164981: <http://trac.webkit.org/changeset/164981>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.