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

Issue 629114 link

Starred by 1 user

Issue metadata

Status: WontFix
Owner:
Closed: Nov 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug-Regression



Sign in to add a comment

1.8% regression in system_health.memory_mobile at 405859:405886

Project Member Reported by petrcermak@chromium.org, Jul 18 2016

Issue description

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

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


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

android-one
Project Member

Comment 3 by 42576172...@developer.gserviceaccount.com, Jul 19 2016

Cc: nyquist@chromium.org
Owner: nyquist@chromium.org

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

Hi nyquist@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 : Add BlimpClientContext and factory with real and dummy implementation
Author  : nyquist
Commit description:
  
The BlimpClientContext is the core class which wires up all of the blimp
functionality. When it's created, it will get all its dependencies from
the embedder. It is currently in charge of creating BlimpContents, but
will also own things like the BlimpClientSession. It is part of the
public //blimp/client/public API. It is a KeyedService and is typically
tied to a Profile.

The BlimpClientContextFactory lives in //chrome to be able to access
Profile during construction, since //blimp/client currently does not
depend on //content where BrowserContext lives, and there are upcoming
dependencies that will be implemented by classes in //chrome.

The BlimpClientContext has two implementations in
//blimp/client/core, an actual one and a dummy implementation. The
dummy implementation is to be used when we do not want the blimp
code to be used, and the core implementation is the real blimp
implementation. Which one of these are in use is controlled by the
GN argument enable_blimp which defaults to false.

The BlimpClientContext has a public static create-method that is not
implemented in //blimp/client/public, but instead it is implemented by
//blimp/client/core by both the actual and the dummy implementation,
so the right method will be chosen at link-time. This means that a
target can only link with either the actual or dummy implementation.
Everything in //blimp/client/core is only visible to //blimp/client/*
and embedders should only ever depend on //blimp/client/public, which
will bring in the correct implementation. This works the same for both
C++ and Java, so when using the BlimpClientContextFactory (either in
Java or C++), the right implementation (actual or dummy) will always
be chosen correctly based on the link-time selection of the
Create-method.

The BlimpClientContext has a delegate that it can call out to whenever
it needs specific functionality from the embedder. As an example, this
CL adds a helper for attaching a Profile to every created
BlimpContents to make have similar functionality as what is built into
WebContents so one can get a Profile and thereby services by only
having access to a BlimpContents.

BUG= 611094 
TBR=erg@chromium.org

Review-Url: https://codereview.chromium.org/2132163002
Cr-Commit-Position: refs/heads/master@{#405860}
Commit  : 9aa0cdee23229ca95a3bea79abcc584a395d016e
Date    : Fri Jul 15 21:21:47 2016


===== TESTED REVISIONS =====
Revision         Mean    Std Dev  N  Good?
chromium@405858  905728  4612.06  8  good
chromium@405859  907776  5334.92  8  good
chromium@405860  916685  3426.96  5  bad    <--
chromium@405862  914227  4486.94  5  bad
chromium@405865  913920  1448.15  8  bad
chromium@405872  918323  4486.94  5  bad
chromium@405886  917504  4096.0   5  bad

Bisect job ran on: android_one_perf_bisect
Bug ID: 629114

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_news-memory:chrome:all_processes:reported_by_os:system_memory:ashmem:private_dirty_size_avg/load_news_hackernews
Relative Change: 1.27%
Score: 99.9

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


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

| 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!
Cc: petrcermak@chromium.org
Perf sheriff ping: reminder to follow up on possible performance issues
Cc: -petrcermak@chromium.org
Owner: petrcermak@chromium.org
Could this be something else or are the perf numbers very clear here?
From looking at the attached charts, the numbers seem to go down again right after.
The numbers are quite clear (#3):

===== TESTED REVISIONS =====
Revision         Mean    Std Dev  N  Good?
chromium@405858  905728  4612.06  8  good
chromium@405859  907776  5334.92  8  good
chromium@405860  916685  3426.96  5  bad    <--
chromium@405862  914227  4486.94  5  bad
chromium@405865  913920  1448.15  8  bad
chromium@405872  918323  4486.94  5  bad
chromium@405886  917504  4096.0   5  bad

but I'm running another bisect to confirm it.

Note that the chart might have gone down for a completely unrelated reason, so this should be investigated (assuming the second bisect confirms the suspicion).
Project Member

Comment 10 by 42576172...@developer.gserviceaccount.com, Aug 25 2016

Owner: nyquist@chromium.org

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

Hi nyquist@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 : Add BlimpClientContext and factory with real and dummy implementation
Author  : nyquist
Commit description:
  
The BlimpClientContext is the core class which wires up all of the blimp
functionality. When it's created, it will get all its dependencies from
the embedder. It is currently in charge of creating BlimpContents, but
will also own things like the BlimpClientSession. It is part of the
public //blimp/client/public API. It is a KeyedService and is typically
tied to a Profile.

The BlimpClientContextFactory lives in //chrome to be able to access
Profile during construction, since //blimp/client currently does not
depend on //content where BrowserContext lives, and there are upcoming
dependencies that will be implemented by classes in //chrome.

The BlimpClientContext has two implementations in
//blimp/client/core, an actual one and a dummy implementation. The
dummy implementation is to be used when we do not want the blimp
code to be used, and the core implementation is the real blimp
implementation. Which one of these are in use is controlled by the
GN argument enable_blimp which defaults to false.

The BlimpClientContext has a public static create-method that is not
implemented in //blimp/client/public, but instead it is implemented by
//blimp/client/core by both the actual and the dummy implementation,
so the right method will be chosen at link-time. This means that a
target can only link with either the actual or dummy implementation.
Everything in //blimp/client/core is only visible to //blimp/client/*
and embedders should only ever depend on //blimp/client/public, which
will bring in the correct implementation. This works the same for both
C++ and Java, so when using the BlimpClientContextFactory (either in
Java or C++), the right implementation (actual or dummy) will always
be chosen correctly based on the link-time selection of the
Create-method.

The BlimpClientContext has a delegate that it can call out to whenever
it needs specific functionality from the embedder. As an example, this
CL adds a helper for attaching a Profile to every created
BlimpContents to make have similar functionality as what is built into
WebContents so one can get a Profile and thereby services by only
having access to a BlimpContents.

BUG= 611094 
TBR=erg@chromium.org

Review-Url: https://codereview.chromium.org/2132163002
Cr-Commit-Position: refs/heads/master@{#405860}
Commit  : 9aa0cdee23229ca95a3bea79abcc584a395d016e
Date    : Fri Jul 15 21:21:47 2016


===== TESTED REVISIONS =====
Revision         Mean    Std Dev  N  Good?
chromium@405858  904704  6359.64  8  good
chromium@405859  903578  3663.57  5  good
chromium@405860  914432  3630.72  8  bad    <--
chromium@405862  915456  3792.16  8  bad
chromium@405865  914432  5250.01  8  bad
chromium@405872  918528  1896.08  8  bad
chromium@405886  914432  3630.72  8  bad

Bisect job ran on: android_one_perf_bisect
Bug ID: 629114

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_news-memory:chrome:all_processes:reported_by_os:system_memory:ashmem:private_dirty_size_avg/load_news_hackernews
Relative Change: 0.72%
Score: 99.5

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


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

| 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!
nyquist: The second bisect also very clearly marked your patch as the culprit. Could you please investigate the issue?
Status: Started (was: Assigned)
Labels: SystemHealth-Sheriff
Labels: -Performance-Sheriff
Labels: OS-Android
Owner: dtrainor@chromium.org
I haven't gotten to the bottom of this yet.

dtrainor: Could you see if you could get someone to investigate this while I'm gone?
Labels: Performance
Components: Mobile>Blimp>Client
Labels: Archive-Blimp

Sign in to add a comment