New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 592282 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Email to this user bounced
Closed: Mar 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

20.8% regression in dromaeo.domcorequery at 379427:379447

Project Member Reported by ericwilligers@chromium.org, Mar 7 2016

Issue description

See the link to graphs below.
 
All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=592282

Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDAv6WCrgkM


Bot(s) for this bug's original alert(s):

chromium-rel-win7-dual
Cc: je_julie...@samsung.com
Owner: je_julie...@samsung.com

=== 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!
Thanks for reporting this issue.
I'll look into it and update the status.
Cc: tkent@chromium.org keishi@chromium.org dmazz...@chromium.org
@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,
results-chart_cur1.json
8.8 KB View Download
results-chart_cur2.json
8.8 KB View Download
results-chart_cur3.json
8.8 KB View Download
results-chart_revert1.json
8.8 KB View Download
results-chart_revert2.json
8.8 KB View Download
results-chart_revert3.json
8.8 KB View Download

Comment 5 by tkent@chromium.org, Mar 13 2016

I suspect CORE_EXPORTs added to NodeTraversal.h decreased DOM traversal performance.


@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.
Status: Fixed (was: Assigned)
@tkent, Thanks for helping me fix this issue.
I updated the status to 'fixed' to get verification.
Status: Assigned (was: Fixed)
@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?

Comment 10 by tkent@chromium.org, Mar 18 2016

Status: Fixed (was: Assigned)
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.

@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