New issue
Advanced search Search tips

Issue 678580 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

1.7% regression in memory.top_10_mobile at 441215:441281

Project Member Reported by primiano@chromium.org, Jan 5 2017

Issue description

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

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


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

android-one
Cc: ccameron@chromium.org
Owner: ccameron@chromium.org

=== PERF REGRESSION ===


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

Hi ccameron@chromium.org, the bisect results pointed to your CL, please take a look at the
results.


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


===== SUSPECTED CL(s) =====
Subject : The great shader refactor: Merge fragment uniforms
Author  : ccameron
Commit description:
  
Move uniform management to the base fragment program, rather than
repeating them in every sub-class.

BUG= 667966 
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2601343002
Cr-Commit-Position: refs/heads/master@{#441249}
Commit  : f9fa543ecdbcf8a51ad0992d943ca77e20d7991c
Date    : Tue Jan 03 23:28:01 2017


===== TESTED REVISIONS =====
Revision         Mean      Std Dev  N   Good?
chromium@441214  12163472  150160   6   good
chromium@441248  12293272  329052   14  good
chromium@441249  12457748  285474   9   bad    <--
chromium@441250  12413918  350955   14  bad
chromium@441251  12451087  182811   9   bad
chromium@441253  12470775  159739   6   bad
chromium@441257  12429488  205745   9   bad
chromium@441265  12438429  190287   9   bad
chromium@441281  12429898  146007   6   bad

Bisect job ran on: android_one_perf_bisect
Bug ID: 678580

Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests --story-filter=http.search.yahoo.com.search..ylt.p.google memory.top_10_mobile
Test Metric: memory:chrome:all_processes:reported_by_chrome:malloc:effective_size_avg/foreground/http_search_yahoo_com_search__ylt_p_google
Relative Change: 2.19%

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_one_perf_bisect/builds/1905
Job details: https://chromeperf.appspot.com/buildbucket_job_status/8991306028853688288


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

| 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!
This will be made up for and then some by https://codereview.chromium.org/2613903002/.
(assuming it's valid)
Cc: sullivan@chromium.org
 Issue 678732  has been merged into this issue.
Got a bunch more overlapping memory alerts; added them to this bug and kicked off bisects just to be sure. We detected 21 regressions, but the largest is 600k
Cc: liber...@chromium.org
Owner: liber...@chromium.org

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

Hi liberato@chromium.org, the bisect results pointed to your CL, please take a look at the
results.


=== BISECT JOB RESULTS ===
Perf regression found with culprit

Suspected Commit
  Author : liberato
  Commit : 7345d7850f8c96020a2a43792b699b70093e9ac4
  Date   : Wed Jan 04 00:56:22 2017
  Subject: Turn off VrShell experiment in testing config.

Bisect Details
  Configuration: android_nexus9_perf_bisect
  Benchmark    : system_health.memory_mobile
  Metric       : memory:chrome:all_processes:reported_by_chrome:v8:effective_size_avg/blank_about/blank_about_blank
  Change       : 8.97% | 4029520.0 -> 4390969.33333

Revision             Result                  N
chromium@441231      4029520 +- 391.918      6      good
chromium@441256      4029467 +- 369.504      6      good
chromium@441268      4029520 +- 391.918      6      good
chromium@441271      4029573 +- 369.504      6      good
chromium@441272      4390439 +- 583.004      6      bad       <--
chromium@441273      4390191 +- 474.693      6      bad
chromium@441274      4390277 +- 600.444      6      bad
chromium@441280      4390895 +- 583.004      6      bad
chromium@441329      4390969 +- 455.286      6      bad

To Run This Test
  src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests --story-filter=blank.about.blank system_health.memory_mobile

Debug Info
  https://chromeperf.appspot.com/buildbucket_job_status/8991216916986188864

Is this bisect wrong?
  https://chromeperf.appspot.com/bad_bisect?try_job_id=5030960406462464


| 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!

=== BISECT JOB RESULTS ===
Perf regression found with culprit

Suspected Commit
  Author : liberato
  Commit : 7345d7850f8c96020a2a43792b699b70093e9ac4
  Date   : Wed Jan 04 00:56:22 2017
  Subject: Turn off VrShell experiment in testing config.

Bisect Details
  Configuration: android_nexus5_perf_bisect
  Benchmark    : system_health.memory_mobile
  Metric       : memory:chrome:all_processes:reported_by_chrome:v8:effective_size_avg/load_search/load_search_google
  Change       : 9.06% | 5300761.33333 -> 5780855.33333

Revision             Result                  N
chromium@440964      5300761 +- 22253.4      6      good
chromium@441184      5325111 +- 18599.3      6      good
chromium@441239      5318328 +- 7942.39      6      good
chromium@441267      5322500 +- 15240.5      6      good
chromium@441271      5322659 +- 17494.8      6      good
chromium@441272      5830889 +- 5921.8       6      bad       <--
chromium@441273      5816437 +- 93286.4      6      bad
chromium@441274      5830895 +- 7371.94      6      bad
chromium@441281      5796573 +- 116107       6      bad
chromium@441294      5831872 +- 4932.55      6      bad
chromium@441403      5780855 +- 128632       6      bad

To Run This Test
  src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests --story-filter=load.search.google system_health.memory_mobile

Debug Info
  https://chromeperf.appspot.com/buildbucket_job_status/8991216938850500928

Is this bisect wrong?
  https://chromeperf.appspot.com/bad_bisect?try_job_id=6456080487415808


| 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!
i'll take a look, but the change in question seems very unlikely to be the root cause.  all use of overlays for video playback in chrome builds without a channel were turned off sometime in December unintentionally.  i turned them back on.

looking at the graph, they've been stable since well before then.
This is almost certainly because of my patch, but it will more-than-recover when https://codereview.chromium.org/2613903002/ lands.

If you check out
https://cs.chromium.org/chromium/src/cc/output/gl_renderer.h?rcl=0&l=443

You'll see that we have, allocated by value, the following pile of shaders
  TileProgram tile_program_[LAST_TEX_COORD_PRECISION + 1][LAST_SAMPLER_TYPE + 1];
  TileProgramOpaque tile_program_opaque_[LAST_TEX_COORD_PRECISION + 1][LAST_SAMPLER_TYPE + 1];
  TileProgramAA tile_program_aa_[LAST_TEX_COORD_PRECISION + 1][LAST_SAMPLER_TYPE + 1];
  TileProgramSwizzle tile_program_swizzle_[LAST_TEX_COORD_PRECISION + 1][LAST_SAMPLER_TYPE + 1];
  TileProgramSwizzleOpaque tile_program_swizzle_opaque_[LAST_TEX_COORD_PRECISION + 1][LAST_SAMPLER_TYPE + 1];
  TileProgramSwizzleAA tile_program_swizzle_aa_[LAST_TEX_COORD_PRECISION + 1][LAST_SAMPLER_TYPE + 1];
  TextureProgram texture_program_[LAST_TEX_COORD_PRECISION + 1][LAST_SAMPLER_TYPE + 1];
  NonPremultipliedTextureProgram nonpremultiplied_texture_program_[LAST_TEX_COORD_PRECISION + 1][LAST_SAMPLER_TYPE + 1];
  TextureBackgroundProgram texture_background_program_[LAST_TEX_COORD_PRECISION + ][LAST_SAMPLER_TYPE + 1];
  NonPremultipliedTextureBackgroundProgram nonpremultiplied_texture_background_program_[LAST_TEX_COORD_PRECISION + 1][LAST_SAMPLER_TYPE + 1];
  RenderPassProgram render_pass_program_[LAST_TEX_COORD_PRECISION + 1][LAST_BLEND_MODE + 1];
  RenderPassProgramAA render_pass_program_aa_[LAST_TEX_COORD_PRECISION + 1][LAST_BLEND_MODE + 1];
  RenderPassMaskProgram render_pass_mask_program_[LAST_TEX_COORD_PRECISION + 1][LAST_SAMPLER_TYPE + 1][LAST_BLEND_MODE + 1][LAST_MASK_VALUE + 1];
  RenderPassMaskProgramAA render_pass_mask_program_aa_[LAST_TEX_COORD_PRECISION + 1][LAST_SAMPLER_TYPE + 1][LAST_BLEND_MODE + 1][LAST_MASK_VALUE + 1];
  RenderPassColorMatrixProgram render_pass_color_matrix_program_[LAST_TEX_COORD_PRECISION + 1][LAST_BLEND_MODE + 1];
  RenderPassColorMatrixProgramAA render_pass_color_matrix_program_aa_[LAST_TEX_COORD_PRECISION + 1][LAST_BLEND_MODE + 1];
  RenderPassMaskColorMatrixProgram render_pass_mask_color_matrix_program_[LAST_TEX_COORD_PRECISION + 1][LAST_SAMPLER_TYPE + 1][LAST_BLEND_MODE + 1][LAST_MASK_VALUE + 1];
  RenderPassMaskColorMatrixProgramAA render_pass_mask_color_matrix_program_aa_[LAST_TEX_COORD_PRECISION + 1][LAST_SAMPLER_TYPE + 1][LAST_BLEND_MODE + 1][LAST_MASK_VALUE + 1];
  VideoYUVProgram video_yuv_program_[LAST_TEX_COORD_PRECISION + 1][LAST_SAMPLER_TYPE + 1][2][2][2];
  VideoStreamTextureProgram video_stream_texture_program_[LAST_TEX_COORD_PRECISION + 1];
  DebugBorderProgram debug_border_program_;
  SolidColorProgram solid_color_program_;
  SolidColorProgramAA solid_color_program_aa_;

Most tabs use <5 of these shaders.

So, what I'm doing in the patch is that I'm merging these all to a common base class, merging their code (cause they all basically do the same thing), and then letting us look them up via some sort of hash table.

When I get a chance, I'll go figure out exactly how many of these we used to allocate, but adding a few hundred bytes to each could definitely get up to 600k.
Issue 679488 has been merged into this issue.
Owner: ccameron@chromium.org
Status: Fixed (was: Untriaged)
https://codereview.chromium.org/2613903002/ landed, and the graphs are back to normal.

I was hoping they'd be better than normal, but I'll settle for normal.
Labels: Performance-Memory

Sign in to add a comment