Issue metadata
Sign in to add a comment
|
20.8% regression in dromaeo.domcorequery at 379427:379447 |
||||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
Mar 7 2016
=== Auto-CCing suspected CL author je_julie.kim@samsung.com === Hi je_julie.kim@samsung.com, the bisect results pointed to your CL below as possibly causing a regression. Please have a look at this info and see whether your CL be related. ===== BISECT JOB RESULTS ===== Status: completed ===== SUSPECTED CL(s) ===== Subject : posinset and setsize for input type, radio, exposed in AX tree Author : je_julie.kim Commit description: posinset and setsize for radio input should be index and size of group and exposed as object attributes. This patch introduces AXRadioInput class to handle input element with radio type. BUG= 557041 Review URL: https://codereview.chromium.org/1628283002 Cr-Commit-Position: refs/heads/master@{#379445} Commit : c52244a6cfc56abce6411a2d82b31eaa06167ec3 Date : Sat Mar 05 02:52:27 2016 ===== TESTED REVISIONS ===== Revision Mean Value Std. Dev. Num Values Good? chromium@379426 9920.700306 602.108833 27 good chromium@379437 9828.18234 174.971511 12 good chromium@379443 9904.717422 525.106201 12 good chromium@379444 9466.930831 425.867995 5 good chromium@379445 10414.789402354.335033 12 bad chromium@379447 10465.54646687.814538 18 bad Bisect job ran on: win_perf_bisect Bug ID: 592282 Test Command: python src/tools/perf/run_benchmark -v --browser=release --output-format=chartjson --also-run-disabled-tests dromaeo.domcorequery Test Metric: dom/dom Relative Change: 5.10% Score: 98.0 Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/win_perf_bisect/builds/6385 Job details: https://chromeperf.appspot.com/buildbucket_job_status/9018880154765499712 | O O | Visit http://www.chromium.org/developers/speed-infra/perf-bug-faq | X | for more information addressing perf regression bugs. For feedback, | / \ | file a bug with label Cr-Tests-AutoBisect. Thank you!
,
Mar 8 2016
Thanks for reporting this issue. I'll look into it and update the status.
,
Mar 12 2016
@ericwilligers, I tried to find relationship between perf regression and my CL for several days. Let me describe my CL briefly. It introduced AXRadioInput class and it's only created with HTMLInput with Radio type. At first time, I thought this regression could be related to AXRadioInput as AXRadioInput has some dom traversing after it's created but I confirmed it's not created while this perf TC is running. Except introducing AXRadioInput, this CL added a small API at HTMLInputElement called by AXRadioInput. It's not called either on running this TC as AXRadioInput is not created. 1) I ran this TC over 50 times on Windows, linux and Mac with enabling AX because my TC is related to AX and it's enabled on Windows by default. But it shows that my CL sometimes seems to affect perf but sometimes not. 2) I put some logs to my changes to check whether it works while running this TC or not with release mode and debug mode, both of them, with changing this option, --browser=release and --browser=debug. But this TC doesn't take my changes. 3) As debug log or printf could sometimes dropped, I added intentional crash code at AXRadioInput to check it's really not created on running this TC and it works without crash. It means that AXRadioInput is not created at all. Because myCL seems to be related to perf degrade from the regression graph at #1, I tried to find connection but I couldn't. I attached my several local test results, results-chart.json. @tkent and keishi, The added API at HTMLInputElement added at that CL could affect performance degrade without creating AXRadioInput? If there is anything I can check, please let me know. Thanks,
,
Mar 13 2016
I suspect CORE_EXPORTs added to NodeTraversal.h decreased DOM traversal performance.
,
Mar 14 2016
@tkent, Thanks a lot for your comment. I didn't realized it could affect performance. I'll run TC again to check if CORE_EXPORT affects perf degrade.
,
Mar 16 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d420eecfc4f7881986bfbc623c3598a0b72c06c9 commit d420eecfc4f7881986bfbc623c3598a0b72c06c9 Author: je_julie.kim <je_julie.kim@samsung.com> Date: Wed Mar 16 00:47:12 2016 Removed CORE_EXPORT from APIs at NodeTraversal. CORE_EXPORT was added at crrev.com/1628283002 and it caused performance degrade on traversing dom. This patch adds nextRadioButtonInGroup at RadioInputType and makes AXRadioInput use it to avoid using dom traversing APIs directly. BUG= 592282 Review URL: https://codereview.chromium.org/1798043002 Cr-Commit-Position: refs/heads/master@{#381371} [modify] https://crrev.com/d420eecfc4f7881986bfbc623c3598a0b72c06c9/third_party/WebKit/Source/core/dom/NodeTraversal.h [modify] https://crrev.com/d420eecfc4f7881986bfbc623c3598a0b72c06c9/third_party/WebKit/Source/core/html/forms/RadioInputType.cpp [modify] https://crrev.com/d420eecfc4f7881986bfbc623c3598a0b72c06c9/third_party/WebKit/Source/core/html/forms/RadioInputType.h [modify] https://crrev.com/d420eecfc4f7881986bfbc623c3598a0b72c06c9/third_party/WebKit/Source/modules/accessibility/AXRadioInput.cpp [modify] https://crrev.com/d420eecfc4f7881986bfbc623c3598a0b72c06c9/third_party/WebKit/Source/modules/accessibility/AXRadioInput.h
,
Mar 16 2016
@tkent, Thanks for helping me fix this issue. I updated the status to 'fixed' to get verification.
,
Mar 17 2016
@tkent,
Sorry to bother you but I found it's not recovered yet on my local Windows System.
I tested with several conditions.
1) current
2) reverted d420eec & c52244a
3) removed calling the APIs for node traversing from AXRadioInput
4) changed nextRadioButtonInGroup to non static.
5) moved nextRadioButtonInGroup to HtmlInputElement.
current revert removedFromAXRadioInput NonStatic HtmlInputElement
getElementsByName__not_in_document_ 1502.2 1551 1451.4 1541.2 1549.6
getElementsByTagName_a_ 88177.2 96329.2 96141.4 93943.4 102817.4
dom 11177.09555 12320.50864 11974.14191 11617.81618 12141.04334
getElementsByTagName__not_in_document_ 122303.2 141484.4 134881.4 130111.6 142976
dom_query 11177.09555 12320.50864 11974.14191 11617.81618 12478.25138
getElementsByTagName_div_ 84046 98936.6 96332 89477.4 102158.6
getElementById 408.1815849 457.9420579 446.5326022 424.1784303 430.3418581
getElementsByTagName_p_ 86570.4 95173.2 95108.4 88482.6 98455.6
getElementById__not_in_document_ 990.4 1046.8 1014.2 1051 1048.4
getElementsByTagName___ 86495.2 97209.2 93716.4 88841.8 103917.8
getElementsByName 660.5356643 704.8589411 691.4617383 652.7386613 682.7918082
I updated document at https://docs.google.com/spreadsheets/d/16uiQEbIcMeH9qG_rHwmaFsw7U5skXqeuMpSqjvLF_ko/edit?usp=sharing
and updated my raw data as well. I tested three times per each case.
The table is from one of result per each case.
As you can see from the table, it's only recovered with moving nextRadioButtonInGroup to HtmlInputElement.
I don't get it clearly why it happens but here is my theory.
HTMLInputElement is already CORE_EXPORT class from the previous version
class CORE_EXPORT HTMLInputElement : public HTMLTextFormControlElement {
but RadioInputType didn't have any CORE_EXPORT API before I add nextRadioButtonInGroup at CL d420eec.
Could it cause perf degrade?
In order to fix it, I don't think HTMLInputElement is not proper position to have nextRadioButtonInGroup.
Do you have any suggestion where to move nextRadioButtonInGroup?
,
Mar 18 2016
According to https://chromeperf.appspot.com/group_report?bug_id=592282, the regression was already recovered by other CLs. I don't think you need to continue working on this.
,
Mar 18 2016
@tkent, thanks for confirm that. I was confused for several days. It seems that my local machine was flaky. I'm bathed in relief. Thanks a lot! |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by ericwilligers@chromium.org
, Mar 7 2016