Optimize HTMLInputElement::updateCheckedRadioButtons

• Component: DOM
• Status: RESOLVED FIXED

[zeng huiqing]

While parsing an Input tag with the 'checked' attribute setted, it will call updateCheckedRadioButtons(), which will lead to traversing the whole document, it is inefficient.

[zeng huiqing]

For page load performance test, we find that while load a page with checkbox be preselected(<input type="checkbox" checked="">), Firefox + IE9 all load much faster than Chromium.[Find that the similar issue has also been reported in http://code.google.com/p/chromium/issues/detail?id=71600]

Through analysis, find the root cause is that when parsing an input element be preselected, the function HTMLInputElement::finishParsingChildren will invoke updateCheckedRadioButtons() which will lead to traversing the whole document, it is inefficient.

And, in my opinion, we may do not need to call updateCheckedRadioButtons() for checkbox in fact, it was just need for 'radio' type. So, I think we may can use 'if Statement' to avoid the unnecessary call for checkbox to remove the traversal overhead.

Of cause, I think in the future we may need to improve the algorithm of updateCheckedRadioButtons(), like to add a structure to save all the input elements' name and type, then we don't have to traverse the whole document:).

[Kent Tamura]

(In reply to comment #1)
> And, in my opinion, we may do not need to call updateCheckedRadioButtons() for checkbox in fact, it was just need for 'radio' type. So, I think we may can use 'if Statement' to avoid the unnecessary call for checkbox to remove the traversal overhead.

Indeed. We don't need to call updateCheckedRadioButtons() for checkboxes.

> Of cause, I think in the future we may need to improve the algorithm of updateCheckedRadioButtons(), like to add a structure to save all the input elements' name and type, then we don't have to traverse the whole document:).

Document::m_formElementsWithState contains all of checkboxes and radio buttons in the document. Searching it for buttons would improve the performance.

[zeng huiqing]

(In reply to comment #2)
Thanks very much for the comment, and I will try to use Document::m_formElementsWithState to improve the updateCheckedRadioButtons():)

[zeng huiqing]

Created attachment 100267 [details]
fix the checkbox issue & improve updateCheckedRadioButtons()

I have added an attachment to fix the bug:
1)in function HTMLInputElement::setChecked(...)
add "if(type()!=(InputTypeNames::checkbox()))' to avoid the unnecessary update for checkbox".

2)in function HTMLInputElement::updateCheckedRadioButtons()
replace traversing document()->body() with Document::m_formElementsWithState to improve the performance.

[Kent Tamura]

(> 2)in function HTMLInputElement::updateCheckedRadioButtons()
> replace traversing document()->body() with Document::m_formElementsWithState to improve the performance.

zeng,
Did you verify it improve the performance?

[WebKit Review Bot]

Comment on attachment 100267 [details]
fix the checkbox issue & improve updateCheckedRadioButtons()

Attachment 100267 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9012415

New failing tests:
http/tests/misc/slow-loading-mask.html
fast/blockflow/japanese-rl-selection.html
svg/transforms/text-with-mask-with-svg-transform.svg
fast/backgrounds/background-leakage.html
fast/box-shadow/scaled-box-shadow.html
fast/backgrounds/repeat/negative-offset-repeat.html
svg/W3C-SVG-1.1/struct-use-01-t.svg
transforms/2d/hindi-rotated.html
fast/forms/interactive-validation-required-checkbox.html
svg/repaint/filter-repaint.svg
fast/blockflow/japanese-lr-selection.html

[WebKit Review Bot]

Created attachment 100281 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Kent Tamura]

Comment on attachment 100267 [details]
fix the checkbox issue & improve updateCheckedRadioButtons()

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

> Source/WebCore/ChangeLog:3
> +        Fix the bug Optimize HTMLInputElement::updateCheckedRadioButtons

nit:
The summary looks a little curious.
"Optimize HTMLInputELement::updateCheckedRadioButton()" is enough.

> Source/WebCore/ChangeLog:14
> +        (WebCore::Document::getFormElements):
> +        * html/HTMLInputElement.cpp:
> +        (WebCore::HTMLInputElement::updateCheckedRadioButtons):
> +        (WebCore::HTMLInputElement::setChecked):

nit:
We had better explanations about what is changed in each of functions.

> Source/WebCore/html/HTMLInputElement.cpp:890
> +    if (type() != (InputTypeNames::checkbox())) {

if (isRadioButton())
would be natural.

> Source/WebCore/html/HTMLInputElement.cpp:892
> +        setNeedsValidityCheck();

setNeedsValidaityCheck() should be called in a case of checkbox.
EWS failure of fast/forms/interactive-validation-required-checkbox.html was caused by this change.

[zeng huiqing]

Created attachment 100433 [details]
new patch

Many thanks for helping review the patch and the good comments:)

1)I have changed 'if(type()!=(InputTypeNames::checkbox()))' to 'if(type()==(InputTypeNames::radio()))', but I'm sorry that I'm not sure that besides 'radio', we don't need to call updateCheckedRadioButtons() for all the other types of input element?

2)The next attachement is the micro-benchmark I use to test the page loading performance, run "./chrome --allow-file-access-from-files omedia_workload4_iterations_rev5x5.html" to get the result, and the result shows that Chromium(build 91994) w/ patch ~1.3x faster to load page radio.html(heavily using 'radio').
My test Platform: 4G i7 860 Ubuntu 10.04

[zeng huiqing]

Created attachment 100434 [details]
testcase

Micro-benchmark for test page loading performance

[Kent Tamura]

(In reply to comment #9)
> Created an attachment (id=100433) [details]
> new patch
> 
> Many thanks for helping review the patch and the good comments:)
> 
> 1)I have changed 'if(type()!=(InputTypeNames::checkbox()))' to 'if(type()==(InputTypeNames::radio()))', but I'm sorry that I'm not sure that besides 'radio', we don't need to call updateCheckedRadioButtons() for all the other types of input element?

Please change it to 'if (isRadioButton())'.  Using string type() is inefficient and strange.
updateCheckedRadioButtons() is necessary only for radio buttons.

> 2)The next attachement is the micro-benchmark I use to test the page loading performance, run "./chrome --allow-file-access-from-files omedia_workload4_iterations_rev5x5.html" to get the result, and the result shows that Chromium(build 91994) w/ patch ~1.3x faster to load page radio.html(heavily using 'radio').

Thank you for the test!
1.3x is a little disappointing. Anyway let's go ahead with this approach, and we should work on further improvement.

[Kent Tamura]

Comment on attachment 100433 [details]
new patch

r- because of type() comparison.

[zeng huiqing]

Created attachment 100439 [details]
newPatch

Thank you for the comments:)
1)I have changed 'if(type()==(InputTypeNames::radio()))' to 'if (isRadioButton())'.
2)Yes, 1.3x is a little disappointing, and I will contiune to try my best for further improvement on updateCheckedRadioButtons().

[Kent Tamura]

Comment on attachment 100439 [details]
newPatch

ok.  Thank you for the patch!

[WebKit Review Bot]

Comment on attachment 100439 [details]
newPatch

Clearing flags on attachment: 100439

Committed r90809: <http://trac.webkit.org/changeset/90809>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.