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

Issue 656947 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

100% improvement in thread_times.simple_mobile_sites at 417088:417296

Project Member Reported by alexclarke@chromium.org, Oct 18 2016

Issue description

Looks like this metric is broken.  Maybe this was on purpose, maybe not.  Lets see what the bisect bot finds. 
 
All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=656947

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


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

android-nexus7v2
Project Member

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

Cc: danakj@chromium.org
Owner: danakj@chromium.org

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

Hi danakj@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 : cc: Merge the DrawLayer()/DrawFrame() and SwapBuffers() methods.
Author  : danakj
Commit description:
  
This deletes the SwapBuffers() method in LayerTreeHostImpl and in
DelegatingRenderer, merging them into DrawLayers() and DrawFrame()
in their respective classes.

Next up is to collapse DelegatingRenderer into LayerTreeHostImpl and
remove all "renderer" and "draw and swap" terminology in
LayerTreeHostImpl, Scheduler, Proxies and friends.

R=enne
BUG= 606056 
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Review-Url: https://codereview.chromium.org/2317953002
Cr-Commit-Position: refs/heads/master@{#417098}
Commit  : aecfcfba7f2622ab3fa1691e939f76ca30a74f2d
Date    : Wed Sep 07 22:35:57 2016


===== TESTED REVISIONS =====
Revision         Mean     Std Dev    N  Good?
chromium@417087  1.94236  0.0400282  5  good
chromium@417094  1.97155  0.030902   5  good
chromium@417096  1.96774  0.0229711  5  good
chromium@417097  1.9952   0.025431   5  good
chromium@417098  0.0      0.0        5  bad    <--
chromium@417101  0.0      0.0        5  bad
chromium@417114  0.0      0.0        5  bad
chromium@417140  0.0      0.0        5  bad
chromium@417192  0.0      0.0        5  bad
chromium@417296  0.0      0.0        5  bad

Bisect job ran on: android_nexus7_perf_bisect
Bug ID: 656947

Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests thread_times.simple_mobile_sites
Test Metric: thread_renderer_compositor_cpu_time_per_frame/thread_renderer_compositor_cpu_time_per_frame
Relative Change: 100.00%
Score: 99.9

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


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

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

It's possible your patch broke the thread_times telemetry benchmark.  Would you mind taking a look?

Comment 5 by danakj@chromium.org, Oct 20 2016

Hm so I deleted the "DelegatingRenderer::SwapBuffers" trace event but I don't see that referred to anywhere in the codebase so probably not that..

Comment 6 by danakj@chromium.org, Oct 20 2016

Cc: enne@chromium.org
Putting this in LTHI gives me non-0 results for thread_times.tough_scrolling_cases:

  TRACE_EVENT0("benchmark", "DelegatingRenderer::SwapBuffers");

Project Member

Comment 7 by bugdroid1@chromium.org, Oct 22 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7607802c76a4a0b826d22807213e981025ec734b

commit 7607802c76a4a0b826d22807213e981025ec734b
Author: danakj <danakj@chromium.org>
Date: Sat Oct 22 02:21:05 2016

cc: Add back a benchmark category trace for counting renderer frames

When deleting DelegatingRenderer::SwapBuffers I failed to notice that
the TRACE_EVENT in there had the benchmark category. This category
is used by perf tests, and without that event they had no way to count
the number of renderer frames produced.

This adds benchmark to the DrawLayers trace event in LayerTreeHostImpl
instead and points the perf tests at that. Also removes the benchmark
category from the GLRenderer and SoftwareRenderer classes as they are
never used in the renderer and are not used for perf tests (tools/ no
longer refers to the name "SwapBuffers" for perf tests).

R=enne@chromium.org
TBR=dtu
BUG= 656947 
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Review-Url: https://chromiumcodereview.appspot.com/2442453003
Cr-Commit-Position: refs/heads/master@{#426952}

[modify] https://crrev.com/7607802c76a4a0b826d22807213e981025ec734b/cc/output/gl_renderer.cc
[modify] https://crrev.com/7607802c76a4a0b826d22807213e981025ec734b/cc/output/software_renderer.cc
[modify] https://crrev.com/7607802c76a4a0b826d22807213e981025ec734b/cc/trees/layer_tree_host_impl.cc
[modify] https://crrev.com/7607802c76a4a0b826d22807213e981025ec734b/tools/perf/metrics/timeline.py

Status: Fixed (was: Untriaged)
It worked thanks! Note I've assigned a "regression" to this bug (it's not a regression), the bisect bot will come and find your patch.  Feel free to ignore that.  I'm going to mark this as fixed.

Sign in to add a comment