Bug 126525 - Move function calls outside loop in dom
Summary: Move function calls outside loop in dom
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
Depends on:
Reported: 2014-01-06 07:28 PST by Laszlo Vidacs
Modified: 2014-03-03 02:12 PST (History)
9 users (show)

See Also:

Patch (3.68 KB, patch)
2014-01-06 07:37 PST, Laszlo Vidacs
no flags Details | Formatted Diff | Diff
Patch (3.02 KB, patch)
2014-02-12 01:42 PST, Laszlo Vidacs
no flags Details | Formatted Diff | Diff
Patch (3.14 KB, patch)
2014-02-24 06:10 PST, Laszlo Vidacs
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Laszlo Vidacs 2014-01-06 07:28:03 PST
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
Comment 1 Laszlo Vidacs 2014-01-06 07:37:53 PST
Created attachment 220435 [details]
Comment 2 Alexey Proskuryakov 2014-01-06 09:37:38 PST
Comment on attachment 220435 [details]

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?
Comment 3 Laszlo Vidacs 2014-01-07 04:04:40 PST
No measurements performed, just omit some unnecessary method calls. I see a possible problem here only when the length is changed within the loop.
Comment 4 Laszlo Vidacs 2014-02-12 01:42:30 PST
Created attachment 223955 [details]
Comment 5 Laszlo Vidacs 2014-02-12 01:43:25 PST
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.
Comment 6 Brent Fulgham 2014-02-19 12:20:30 PST
Comment on attachment 223955 [details]

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.
Comment 7 Laszlo Vidacs 2014-02-24 06:10:57 PST
Created attachment 225058 [details]
Comment 8 Laszlo Vidacs 2014-02-24 06:12:43 PST
Thanks for the feedback, I tried the following code:
for (const Attribute& i : other.m_attributeArray)
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().
Comment 9 WebKit Commit Bot 2014-03-03 02:12:17 PST
Comment on attachment 225058 [details]

Clearing flags on attachment: 225058

Committed r164981: <http://trac.webkit.org/changeset/164981>
Comment 10 WebKit Commit Bot 2014-03-03 02:12:22 PST
All reviewed patches have been landed.  Closing bug.