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

Issue 631041 link

Starred by 2 users

Issue metadata

Status: Duplicate
Owner:
please use my google.com address
Closed: May 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

4.5% regression in thread_times.key_silk_cases at 407262:407289

Project Member Reported by primiano@chromium.org, Jul 25 2016

Issue description

This might be just noise. Trying a bisect.
 
All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=631041

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


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

android-nexus5
Project Member

Comment 4 by 42576172...@developer.gserviceaccount.com, Jul 25 2016

Cc: roc...@chromium.org
Owner: roc...@chromium.org

=== Auto-CCing suspected CL author rockot@chromium.org ===

Hi rockot@chromium.org, 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 : Support early associated interface binding on ChannelMojo
Author  : rockot
Commit description:
  
Changes the associated bindings implementation for ChannelMojo
such that remote interfaces can be acquired immediately upon
ChannelMojo construction rather than having to wait for connection
on the IO thread.

Simplifies the Channel bootstrapping process, removing a round-trip
Init message (and in fact the entire IPC::mojom::Boostrap interface)
since there's no need to actually exchange associated interface handles
over the pipe. Instead both sides can assume the other will use a fixed,
reserved endpoint ID for their IPC::mojom::Channel interface.

This also removes the restriction that associated interfaces must be
added to a Channel after Init. Instead the same constraints apply as
with AddFilter: an associated interface, like a filter, may be added
at any time as long as either Init hasn't been called OR the remote
process hasn't been launched.

The result of this CL is that any place it's safe to AddFilter,
it's also safe to AddAssociatedInterface; and any place it's safe to
Send, it's also safe to GetRemoteAssociatedInterface and begin using
any such remote interface immediately.

Remote interface requests as well as all messages to remote interfaces
retain FIFO with respect to any Send calls on the same thread. Local
interface request dispatch as well as all messages on locally bound
associated interfaces retain FIFO with respect to any OnMessageReceived
calls on the same thread.

BUG=612500, 619202 

Committed: https://crrev.com/e1037f997da9e1d44ca3b09d4ff32f0465673091
Committed: https://crrev.com/508da24622f957a01b076ccd058bfdccc79068a4
Review-Url: https://codereview.chromium.org/2163633003
Cr-Original-Original-Commit-Position: refs/heads/master@{#406720}
Cr-Original-Commit-Position: refs/heads/master@{#407050}
Cr-Commit-Position: refs/heads/master@{#407264}
Commit  : 0e4de5f9a519c6cd206448a10eccc7a535e3db64
Date    : Fri Jul 22 21:20:12 2016


===== TESTED REVISIONS =====
Revision         Mean     Std Dev    N  Good?
chromium@407261  3.1706   0.0249109  5  good
chromium@407263  3.18897  0.0221303  5  good
chromium@407264  3.35917  0.0199469  5  bad    <--
chromium@407265  3.36975  0.0294314  5  bad
chromium@407268  3.35683  0.0227606  5  bad
chromium@407275  3.36685  0.0261529  5  bad
chromium@407289  3.35622  0.0143797  5  bad

Bisect job ran on: android_nexus5_perf_bisect
Bug ID: 631041

Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --also-run-disabled-tests thread_times.key_silk_cases
Test Metric: thread_IO_cpu_time_per_frame/thread_IO_cpu_time_per_frame
Relative Change: 5.85%
Score: 99.9

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_nexus5_perf_bisect/builds/3865
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9006155527787323904


Not what you expected? We'll investigate and get back to you!
  https://chromeperf.appspot.com/bad_bisect?try_job_id=5793154419130368

| 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 component Tests>AutoBisect.  Thank you!
Project Member

Comment 5 by 42576172...@developer.gserviceaccount.com, Jul 25 2016


===== BISECT JOB RESULTS =====
Status: completed


===== SUSPECTED CL(s) =====
Subject : Support early associated interface binding on ChannelMojo
Author  : rockot
Commit description:
  
Changes the associated bindings implementation for ChannelMojo
such that remote interfaces can be acquired immediately upon
ChannelMojo construction rather than having to wait for connection
on the IO thread.

Simplifies the Channel bootstrapping process, removing a round-trip
Init message (and in fact the entire IPC::mojom::Boostrap interface)
since there's no need to actually exchange associated interface handles
over the pipe. Instead both sides can assume the other will use a fixed,
reserved endpoint ID for their IPC::mojom::Channel interface.

This also removes the restriction that associated interfaces must be
added to a Channel after Init. Instead the same constraints apply as
with AddFilter: an associated interface, like a filter, may be added
at any time as long as either Init hasn't been called OR the remote
process hasn't been launched.

The result of this CL is that any place it's safe to AddFilter,
it's also safe to AddAssociatedInterface; and any place it's safe to
Send, it's also safe to GetRemoteAssociatedInterface and begin using
any such remote interface immediately.

Remote interface requests as well as all messages to remote interfaces
retain FIFO with respect to any Send calls on the same thread. Local
interface request dispatch as well as all messages on locally bound
associated interfaces retain FIFO with respect to any OnMessageReceived
calls on the same thread.

BUG=612500, 619202 

Committed: https://crrev.com/e1037f997da9e1d44ca3b09d4ff32f0465673091
Committed: https://crrev.com/508da24622f957a01b076ccd058bfdccc79068a4
Review-Url: https://codereview.chromium.org/2163633003
Cr-Original-Original-Commit-Position: refs/heads/master@{#406720}
Cr-Original-Commit-Position: refs/heads/master@{#407050}
Cr-Commit-Position: refs/heads/master@{#407264}
Commit  : 0e4de5f9a519c6cd206448a10eccc7a535e3db64
Date    : Fri Jul 22 21:20:12 2016


===== TESTED REVISIONS =====
Revision         Mean     Std Dev     N  Good?
chromium@407261  3.17763  0.0138453   5  good
chromium@407263  3.20936  0.00871233  5  good
chromium@407264  3.32234  0.0303387   5  bad    <--
chromium@407265  3.33814  0.0291247   5  bad
chromium@407268  3.35811  0.0140334   5  bad
chromium@407275  3.3696   0.0373151   5  bad
chromium@407289  3.35586  0.0293602   5  bad

Bisect job ran on: android_nexus5_perf_bisect
Bug ID: 631041

Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --also-run-disabled-tests thread_times.key_silk_cases
Test Metric: thread_IO_cpu_time_per_frame/thread_IO_cpu_time_per_frame
Relative Change: 5.61%
Score: 99.9

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_nexus5_perf_bisect/builds/3866
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9006155532613575424


Not what you expected? We'll investigate and get back to you!
  https://chromeperf.appspot.com/bad_bisect?try_job_id=5992423117815808

| 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 component Tests>AutoBisect.  Thank you!

Comment 6 by roc...@chromium.org, Jul 26 2016

The bisected CL was reverted a few times without any noticeable impact on the graph.
Started another couple of bisects.

>The bisected CL was reverted a few times without any noticeable impact on the graph.
CLs get batched by the perf waterfall. Are you sure that the (un)reverts didn't get grouped in the same batch?
Project Member

Comment 10 by 42576172...@developer.gserviceaccount.com, Jul 27 2016


===== BISECT JOB RESULTS =====
Status: completed


===== SUSPECTED CL(s) =====
Subject : Support early associated interface binding on ChannelMojo
Author  : rockot
Commit description:
  
Changes the associated bindings implementation for ChannelMojo
such that remote interfaces can be acquired immediately upon
ChannelMojo construction rather than having to wait for connection
on the IO thread.

Simplifies the Channel bootstrapping process, removing a round-trip
Init message (and in fact the entire IPC::mojom::Boostrap interface)
since there's no need to actually exchange associated interface handles
over the pipe. Instead both sides can assume the other will use a fixed,
reserved endpoint ID for their IPC::mojom::Channel interface.

This also removes the restriction that associated interfaces must be
added to a Channel after Init. Instead the same constraints apply as
with AddFilter: an associated interface, like a filter, may be added
at any time as long as either Init hasn't been called OR the remote
process hasn't been launched.

The result of this CL is that any place it's safe to AddFilter,
it's also safe to AddAssociatedInterface; and any place it's safe to
Send, it's also safe to GetRemoteAssociatedInterface and begin using
any such remote interface immediately.

Remote interface requests as well as all messages to remote interfaces
retain FIFO with respect to any Send calls on the same thread. Local
interface request dispatch as well as all messages on locally bound
associated interfaces retain FIFO with respect to any OnMessageReceived
calls on the same thread.

BUG=612500, 619202 

Committed: https://crrev.com/e1037f997da9e1d44ca3b09d4ff32f0465673091
Committed: https://crrev.com/508da24622f957a01b076ccd058bfdccc79068a4
Review-Url: https://codereview.chromium.org/2163633003
Cr-Original-Original-Commit-Position: refs/heads/master@{#406720}
Cr-Original-Commit-Position: refs/heads/master@{#407050}
Cr-Commit-Position: refs/heads/master@{#407264}
Commit  : 0e4de5f9a519c6cd206448a10eccc7a535e3db64
Date    : Fri Jul 22 21:20:12 2016


===== TESTED REVISIONS =====
Revision         Mean     Std Dev  N  Good?
chromium@407217  2092.34  2.56977  5  good
chromium@407244  2091.45  4.53858  5  good
chromium@407258  2090.93  2.08971  5  good
chromium@407262  2091.74  2.30197  5  good
chromium@407263  2092.44  3.87619  5  good
chromium@407264  2213.29  1.77463  5  bad    <--
chromium@407265  2214.56  3.09895  5  bad
chromium@407271  2209.8   2.71673  5  bad
chromium@407324  2210.87  2.09887  5  bad

Bisect job ran on: android_nexus6_perf_bisect
Bug ID: 631041

Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --also-run-disabled-tests thread_times.key_idle_power_cases
Test Metric: tasks_per_second_total_all/request-animation-frame.html
Relative Change: 5.66%
Score: 99.9

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_nexus6_perf_bisect/builds/2360
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9005983083391197872


Not what you expected? We'll investigate and get back to you!
  https://chromeperf.appspot.com/bad_bisect?try_job_id=5781713163124736

| 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 component Tests>AutoBisect.  Thank you!
Project Member

Comment 11 by 42576172...@developer.gserviceaccount.com, Jul 27 2016


===== BISECT JOB RESULTS =====
Status: completed


===== SUSPECTED CL(s) =====
Subject : Support early associated interface binding on ChannelMojo
Author  : rockot
Commit description:
  
Changes the associated bindings implementation for ChannelMojo
such that remote interfaces can be acquired immediately upon
ChannelMojo construction rather than having to wait for connection
on the IO thread.

Simplifies the Channel bootstrapping process, removing a round-trip
Init message (and in fact the entire IPC::mojom::Boostrap interface)
since there's no need to actually exchange associated interface handles
over the pipe. Instead both sides can assume the other will use a fixed,
reserved endpoint ID for their IPC::mojom::Channel interface.

This also removes the restriction that associated interfaces must be
added to a Channel after Init. Instead the same constraints apply as
with AddFilter: an associated interface, like a filter, may be added
at any time as long as either Init hasn't been called OR the remote
process hasn't been launched.

The result of this CL is that any place it's safe to AddFilter,
it's also safe to AddAssociatedInterface; and any place it's safe to
Send, it's also safe to GetRemoteAssociatedInterface and begin using
any such remote interface immediately.

Remote interface requests as well as all messages to remote interfaces
retain FIFO with respect to any Send calls on the same thread. Local
interface request dispatch as well as all messages on locally bound
associated interfaces retain FIFO with respect to any OnMessageReceived
calls on the same thread.

BUG=612500, 619202 

Committed: https://crrev.com/e1037f997da9e1d44ca3b09d4ff32f0465673091
Committed: https://crrev.com/508da24622f957a01b076ccd058bfdccc79068a4
Review-Url: https://codereview.chromium.org/2163633003
Cr-Original-Original-Commit-Position: refs/heads/master@{#406720}
Cr-Original-Commit-Position: refs/heads/master@{#407050}
Cr-Commit-Position: refs/heads/master@{#407264}
Commit  : 0e4de5f9a519c6cd206448a10eccc7a535e3db64
Date    : Fri Jul 22 21:20:12 2016


===== TESTED REVISIONS =====
Revision         Mean     Std Dev  N  Good?
chromium@407217  2154.76  1.94667  5  good
chromium@407244  2154.55  5.00942  5  good
chromium@407258  2153.75  2.75185  5  good
chromium@407262  2155.76  3.53185  5  good
chromium@407263  2155.04  3.19265  5  good
chromium@407264  2274.26  4.14445  5  bad    <--
chromium@407265  2273.97  5.1649   5  bad
chromium@407271  2277.28  6.78535  5  bad
chromium@407324  2272.95  5.58051  5  bad

Bisect job ran on: android_nexus6_perf_bisect
Bug ID: 631041

Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --also-run-disabled-tests thread_times.key_idle_power_cases
Test Metric: tasks_per_second_total_all/set-timeout.html
Relative Change: 5.49%
Score: 99.9

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_nexus6_perf_bisect/builds/2359
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9005983087964020560


Not what you expected? We'll investigate and get back to you!
  https://chromeperf.appspot.com/bad_bisect?try_job_id=5802607667314688

| 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 component Tests>AutoBisect.  Thank you!
Hmm seems clearly linked. Oddly enough though r407264 was already landed exactly as-is in r407050. It was reverted in r407120.

There are no noticeable changes in these graphs during that window, though perhaps the window is too small?
Yeah 5 bisects (3 here and 2 on  Issue 631423 ) pointed to this change. I think somehow this is related. 
Ping rockot@, any updating on investigating this regression?
No, I don't have any updates.

It's not surprising that this change would have some impact here, but I'm not sure what should be done about it at the moment if anything. There are no obvious improvements to make to the code (though it has been simplified some since this CL landed), and in general it has to somewhat increase the complexity of IPC Channel behavior to get us some functionality that we require moving forward.

Do we have any tools that help us understand regressions like this better? e.g. it would be really nice if instead of just "X% more CPU time per frame" we could get detailed profiling data to compare good vs bad revisions.

You can click the "trace" link on the tooltip on any of the charts to get the trace that the metrics are calculated from. Example from ChromiumPerf/android-nexus6/thread_times.key_idle_power_cases/tasks_per_second_total_all/request-animation-frame.html:

Before: https://console.developers.google.com/m/cloudstorage/b/chrome-telemetry-output/o/trace-file-id_3-2016-07-22_14-08-29-41713.html
After: https://console.developers.google.com/m/cloudstorage/b/chrome-telemetry-output/o/trace-file-id_3-2016-07-22_20-14-13-32411.html
Perf sheriff ping: reminder to follow up on possible performance issues
Weekly ping from your friendly perf regression triage team.
Perf sheriff ping
Project Member

Comment 20 by 42576172...@developer.gserviceaccount.com, Oct 20 2016

Bisect failed: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_nexus6_perf_bisect/builds/2679
Failure reason: the build has failed.
Additional errors:
The metric was not found in the test output.
Either of the initial "good" or "bad" revisions failed to be tested or built.

Mergedinto: 627082
Status: Duplicate (was: Assigned)

Sign in to add a comment