New issue
Advanced search Search tips

Issue 633182 link

Starred by 1 user
Status: WontFix
Owner:
dcheng@chromium.org
Closed: May 2017
Cc:
dcheng@chromium.org
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

2.8% regression in system_health.memory_mobile at 408618:408645

Project Member Reported by petrcermak@chromium.org, Aug 1 2016 
See the link to graphs below.
Project Member

Comment 5 by 42576172...@developer.gserviceaccount.com, Aug 2 2016 

Bisect failed: Unknown
Failure reason: the build has failed due to infrastructure failure.
Project Member

Comment 7 by 42576172...@developer.gserviceaccount.com, Aug 6 2016

Cc: toyoshim@chromium.org
Owner: toyoshim@chromium.org

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

Hi toyoshim@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 : Reland: Web MIDI: use mojom::blink::PermissionService directly to ask permission
Author  : toyoshim
Commit description:
  
Changes from the first attempt:

- AwPermissionManager::RequestPermissions() was implemented for
  Android WebView in a separate CL.
- Workarounds to avoid touching NOTIMPLEMENTED() was removed

Original description follows:

SystemWebViewShellLayoutTest failed because of the same reason with
other modified tests in this CL. Tests are modified to pass, but now
it does not cover {sysex:true} case that needs
AwPermissionManager::RequestPermissions implementation.
It will be implemented in the next CL soon.

Original description follows:
Web MIDI asked permissions via public/web interfaces.
But now that PermissionService is available in Blink,
use the service to ask permissions.

This migration makes it possible to remove all MIDI
related public/web interfaces.

BUG=582328

Committed: https://crrev.com/15c1a1ffd1f4c620b89191209dc76ef5557dd8fc
Cr-Commit-Position: refs/heads/master@{#404136}

TEST=${OUT}/bin/run_system_webview_shell_layout_test_apk # with a built SystemWebViewGoogle.apk
TEST=git cl try

Review-Url: https://codereview.chromium.org/2116763002
Cr-Commit-Position: refs/heads/master@{#408621}
Commit  : 8c21bb977b580edb009eb2fe0d3f511fd5ca42f4
Date    : Fri Jul 29 12:28:28 2016


===== TESTED REVISIONS =====
Revision         Mean    Std Dev  N  Good?
chromium@408616  864256  0.0      5  good
chromium@408619  864256  0.0      5  good
chromium@408620  864256  0.0      5  good
chromium@408621  875725  1831.79  5  bad    <--
chromium@408626  884736  0.0      5  bad
chromium@408635  884736  0.0      5  bad

Bisect job ran on: android_s5_perf_bisect
Bug ID: 633182

Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --also-run-disabled-tests system_health.memory_mobile
Test Metric: load_games-memory:chrome:all_processes:reported_by_os:system_memory:ashmem:private_dirty_size_avg/load_games_lazors
Relative Change: 2.37%
Score: 99.9

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_s5_perf_bisect/builds/893
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9005164679408527136


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

| 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 9 by toyoshim@chromium.org, Aug 8 2016 

Let me run another bisect.
This result comes from the second bisect, and the first bisect seems to have suspected another CL.

My CL was a cleanup to remove about 700 lines of source code and some class implementation.
So, it wasn't expected to increase memory usage and binary size.

Comment 10 by petrcermak@chromium.org, Aug 8 2016 

toyoshim: Thanks. It looks like your CL unfortunately hit some less common case in one of the 5 runs (which is why the standard deviation is non-zero). I'd say that the regression is definitely between r408621 and r408626 (inclusive): https://chromium.googlesource.com/chromium/src/+log/8c21bb977b580edb009eb2fe0d3f511fd5ca42f4%5E..c6dbdbb5994554bcb90de049a61a1ae1c73d13cd. If I had to guess, I'd say it's either the Skia or V8 roll.

Comment 11 by toyoshim@chromium.org, Aug 8 2016

Owner: petrcermak@chromium.org
By the way, now the dashboard page says
> Minimum revision range for 5 alerts: 408626:408632

Then, my last bisect is running between this range, and current status is like this;
> chromium@408625  880640  0.0      5  good
> chromium@408632  888832  0.0      5  bad

This is out of range you said at #10, but still we may see a regression in this range.
Can I assign you again?

Comment 12 by petrcermak@chromium.org, Aug 8 2016 

toyoshim: Yes, I'll investigate further. Could you please send me links to results of "the first bisect" from #9 and "[your] last bisect] from #11? This issue only contains results from a single bisect (#7).

Comment 13 by toyoshim@chromium.org, Aug 8 2016 

The bisects I said in the #9 and #11 were the same one.
You can find it as the third bisect starting at 2016-08-08 11:00:17 in the dashboard page, https://chromeperf.appspot.com/group_report?bug_id=633182

Now the updated status is 
===== TESTED REVISIONS =====
Revision         Mean    Std Dev  N  Good?
chromium@408625  880640  0.0      5  good
chromium@408629  888832  0.0      5  bad
chromium@408632  888832  0.0      5  bad

Comment 14 by petrcermak@chromium.org, Aug 8 2016 

toyoshim: Thanks for the explanation. It seems to me that there are more than one smaller regressions in the whole range:

chromium@408616  864256  0.0
chromium@408619  864256  0.0
chromium@408620  864256  0.0
...                           <-- Regression 1 (+15 KiB)
chromium@408625  880640  0.0
chromium@408626  888832  0.0  <-- Regression 2 (+8 KiB)
chromium@408627  888832  0.0
chromium@408629  888832  0.0
chromium@408632  888832  0.0

I suspect the most recent benchmark will blame r408626. I'll then file another bug and start another bisect for the other regression.
Project Member

Comment 15 by 42576172...@developer.gserviceaccount.com, Aug 9 2016 


===== BISECT JOB RESULTS =====
Status: failed


===== TESTED REVISIONS =====
Revision         Mean    Std Dev  N  Good?
chromium@408625  880640  0.0      5  good
chromium@408626  888832  0.0      5  bad
chromium@408627  888832  0.0      5  bad
chromium@408629  888832  0.0      5  bad
chromium@408632  888832  0.0      5  bad

Bisect job ran on: android_nexus7_perf_bisect
Bug ID: 633182

Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --also-run-disabled-tests system_health.memory_mobile
Test Metric: load_games-memory:chrome:all_processes:reported_by_os:system_memory:ashmem:private_dirty_size_avg/load_games_lazors
Relative Change: 0.93%
Score: 0

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_nexus7_perf_bisect/builds/3165
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9004927347214941408


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

| 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 17 by 42576172...@developer.gserviceaccount.com, Aug 9 2016 


===== BISECT JOB RESULTS =====
Status: failed


===== TESTED REVISIONS =====
Revision         Mean    Std Dev  N  Good?
chromium@408625  880640  0.0      5  good
chromium@408626  888832  0.0      5  bad
chromium@408627  888832  0.0      5  bad
chromium@408629  888832  0.0      5  bad
chromium@408632  888832  0.0      5  bad

Bisect job ran on: android_nexus7_perf_bisect
Bug ID: 633182

Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --also-run-disabled-tests system_health.memory_mobile
Test Metric: load_games-memory:chrome:all_processes:reported_by_os:system_memory:ashmem:private_dirty_size_avg/load_games_lazors
Relative Change: 0.93%
Score: 0

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_nexus7_perf_bisect/builds/3165
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9004927347214941408


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

| 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 18 by petrcermak@chromium.org, Aug 9 2016

Cc: -toyoshim@chromium.org
Owner: dcheng@chromium.org
dcheng: The bisect blamed your patch (https://codereview.chromium.org/2191533003) for the regression (+8 KiB in ashmem on http://www8.games.mobi/games/html5/lazors/lazors.html). Please investigate and decide on further action.

Comment 19 by dcheng@chromium.org, Aug 9 2016 

I went through my patch line by line and I honestly can't see how it would have affected memory consumption. It doesn't change any functionality or add any new members. The same task runners that were referenced and kept alive after my patch are the same task runners that would have been referenced and kept alive before my patch. Is there any help here to help profile the memory differences?
Project Member

Comment 22 by 42576172...@developer.gserviceaccount.com, Aug 9 2016

Cc: dcheng@chromium.org

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

Hi dcheng@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 : Refactor Timer classes in preparation for landing FrameTimers.
Author  : dcheng
Commit description:
  
- The original Timer template is now TaskRunnerTimer and requires a
  TaskRunner argument.
- A new Timer template implements the behavior of the original Timer
  template to avoid breaking a lot of code.
- The timer callback parameter has changed to be a TimerBase*, to make
  it easier to share the underlying template implementations. This is a
  safe change, because almost all TaskRunnerTimer callbacks don't use
  the parameter. The few callbacks that care use it to multiplex
  multiple timers in one callback, and only use it for equality
  comparisons with subclasses of TimerBase, so there's no problem.

Followup CLs will introduce frame-specific timers (e.g. LoadingTimer)
which will use TaskRunnerTimer as the base template, as well as rename
the new Timer template to DefaultThreadTimer.

BUG= 624694 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2191533003
Cr-Commit-Position: refs/heads/master@{#408626}
Commit  : c6dbdbb5994554bcb90de049a61a1ae1c73d13cd
Date    : Fri Jul 29 13:25:36 2016


===== TESTED REVISIONS =====
Revision         Mean    Std Dev  N  Good?
chromium@408625  880640  0.0      5  good
chromium@408626  888832  0.0      5  bad    <--
chromium@408627  888832  0.0      5  bad
chromium@408629  888832  0.0      3  bad
chromium@408632  888832  0.0      6  bad

Bisect job ran on: android_nexus6_perf_bisect
Bug ID: 633182

Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --also-run-disabled-tests system_health.memory_mobile
Test Metric: load_games-memory:chrome:all_processes:reported_by_os:system_memory:ashmem:private_dirty_size_avg/load_games_lazors
Relative Change: 0.93%
Score: 99.9

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


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

| 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 24 by 42576172...@developer.gserviceaccount.com, Aug 10 2016 


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


===== SUSPECTED CL(s) =====
Subject : Refactor Timer classes in preparation for landing FrameTimers.
Author  : dcheng
Commit description:
  
- The original Timer template is now TaskRunnerTimer and requires a
  TaskRunner argument.
- A new Timer template implements the behavior of the original Timer
  template to avoid breaking a lot of code.
- The timer callback parameter has changed to be a TimerBase*, to make
  it easier to share the underlying template implementations. This is a
  safe change, because almost all TaskRunnerTimer callbacks don't use
  the parameter. The few callbacks that care use it to multiplex
  multiple timers in one callback, and only use it for equality
  comparisons with subclasses of TimerBase, so there's no problem.

Followup CLs will introduce frame-specific timers (e.g. LoadingTimer)
which will use TaskRunnerTimer as the base template, as well as rename
the new Timer template to DefaultThreadTimer.

BUG= 624694 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2191533003
Cr-Commit-Position: refs/heads/master@{#408626}
Commit  : c6dbdbb5994554bcb90de049a61a1ae1c73d13cd
Date    : Fri Jul 29 13:25:36 2016


===== TESTED REVISIONS =====
Revision         Mean    Std Dev  N  Good?
chromium@408625  880640  0.0      5  good
chromium@408626  888832  0.0      5  bad    <--
chromium@408627  888832  0.0      5  bad
chromium@408629  888832  0.0      5  bad
chromium@408632  888832  0.0      5  bad

Bisect job ran on: android_nexus7_perf_bisect
Bug ID: 633182

Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --also-run-disabled-tests system_health.memory_mobile
Test Metric: load_games-memory:chrome:all_processes:reported_by_os:system_memory:ashmem:private_dirty_size_avg/load_games_lazors
Relative Change: 0.93%
Score: 99.9

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_nexus7_perf_bisect/builds/3170
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9004927347214941408


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

| 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 25 by petrcermak@chromium.org, Aug 10 2016 

dcheng: A separate bisect on a different device (#22) confirmed that the regression was most likely caused by r408626. To run the benchmark locally (with ChromePublic.apk):

$ tools/perf/run_benchmark system_health.memory_mobile --browser=android-chromium --device=android --story-filter=lazors --also-run-disabled-tests -v

Comment 26 by dcheng@chromium.org, Aug 16 2016 

So I really have no idea how to proceed on this investigation.

- How do I manually run this memory test? As of this moment, I have no idea what parts of the system it's exercising.
- How do I use the memory traces that are linked to? I looked at them, and I'm honestly not sure what information I can get out of them. Is there a non-default view that will show memory profiling information?
- After reading over my CL, I'm still having trouble understanding how what is essentially a refactoring only CL can possibly affect this result.

Comment 27 by petrcermak@chromium.org, Aug 17 2016 

> - How do I manually run this memory test? As of this moment, I have no idea
> what parts of the system it's exercising.

What exactly do you mean by "manually run"? You can capture a trace using
chrome://tracing (enable disabled-by-default-memory-infra) and then get the
number by running:

  third_party/catapult/tracing/bin/run_metric <TRACE> memoryMetric

I'm sorry that it's so complicated.

> - How do I use the memory traces that are linked to? I looked at them, and
> I'm honestly not sure what information I can get out of them. Is there a
> non-default view that will show memory profiling information?

You can see the memory stats by clicking on the pink/purple circle containing a
white "M" in one of the top tracks.

To see the breakdown of ashmem:private_dirty_size for a given process, click
on any of the cells with blue font and than navigate down to Total → Android →
Ashmem. The attached screenshot shows private dirty size of ashmem in the
renderer process.

> - After reading over my CL, I'm still having trouble understanding how what is
> essentially a refactoring only CL can possibly affect this result.

I went through your patch and I don't understand it either. I suspect it could
be due to something like a compiler-level optimization. If you feel that your
change is worth the +8 KiB, then I suggest you mark this as WontFix.
ashmem_private_dirty.png
134 KB View Download

Comment 28 by petrcermak@chromium.org, Aug 30 2016

Labels: SystemHealth-Sheriff

Comment 29 by petrcermak@chromium.org, Aug 30 2016

Labels: -Performance-Sheriff

Comment 30 by rbyers@chromium.org, Nov 18 2016

Labels: Performance

Comment 31 by benhenry@chromium.org, May 6 2017

Status: WontFix (was: Assigned)
This regression has been included in a stable release and that stable channel is now deprecated. I'm closing this so that we won't have unresponsive performance regressions in the future.
►

Sign in to add a comment