Bug 62840

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:).

(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.

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

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.

(> 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?

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

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

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.

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

Created attachment 100434 [details] testcase Micro-benchmark for test page loading performance

(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.

Comment on attachment 100433 [details] new patch r- because of type() comparison.

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().

Comment on attachment 100439 [details] newPatch ok. Thank you for the patch!

Comment on attachment 100439 [details] newPatch Clearing flags on attachment: 100439 Committed r90809: <http://trac.webkit.org/changeset/90809>

All reviewed patches have been landed. Closing bug.