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

Issue 647363 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

10.4% regression in tracing.tracing_with_debug_overhead at 417992:418036

Project Member Reported by primiano@chromium.org, Sep 15 2016

Issue description

I believe that this is https://codereview.chromium.org/2328523004
this metric is measuring the spam-level of tracing
 
All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=647363

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


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

chromium-rel-mac-retina
Owner: vmp...@chromium.org
vmpstr, I fear this might be really  https://codereview.chromium.org/2328523004

Comment 4 by vmp...@chromium.org, Sep 15 2016

Hmm.. That's unfortunate. What's our policy on disabled-by-default spam? I'm OK with reverting the patch, but I would also like to understand what's acceptable and what's not in terms of adding traces that might run frequently. 
Project Member

Comment 5 by 42576172...@developer.gserviceaccount.com, Sep 15 2016

Cc: vmp...@chromium.org

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

Hi vmpstr@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/images: Add more disabled-by-default traces to GPU image controller.
Author  : vmpstr
Commit description:
  
This patch adds traces to GpuImageDecodeController in an effort to
better understand the performance characteristics of the code. These
traces are disabled by default.

R=ericrk@chromium.org, enne@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/1fc46747f05525b9565e223f807e2b9fcc38c327
Review-Url: https://codereview.chromium.org/2328523004
Cr-Original-Commit-Position: refs/heads/master@{#417651}
Cr-Commit-Position: refs/heads/master@{#418007}
Commit  : d3aae3acb0950a4438b058abf70fa0b6fbaf93f5
Date    : Mon Sep 12 19:42:47 2016


===== TESTED REVISIONS =====
Revision         Mean     Std Dev  N  Good?
chromium@417991  20029.0  437.4    5  good
chromium@418003  20179.8  427.094  5  good
chromium@418006  20020.2  325.954  5  good
chromium@418007  22105.2  291.578  5  bad    <--
chromium@418008  22799.4  1380.66  5  bad
chromium@418009  22077.6  220.744  5  bad
chromium@418014  22117.8  394.954  5  bad
chromium@418036  21940.8  600.978  5  bad

Bisect job ran on: mac_retina_perf_bisect
Bug ID: 647363

Test Command: src/tools/perf/run_benchmark -v --browser=release --output-format=chartjson --upload-results --also-run-disabled-tests tracing.tracing_with_debug_overhead
Test Metric: Max number of events per second_max/Max number of events per second_max
Relative Change: 9.55%
Score: 98.0

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/mac_retina_perf_bisect/builds/1662
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9001428349052411728


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

| 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: nduca@chromium.org fmea...@chromium.org
Good question, +fmeawad +nduca

Comment 7 by nduca@chromium.org, Sep 16 2016

Cc: zh...@chromium.org
Vlad, a basic question here is "what is the target time and overhead for which frameviewer should operate?" I think frameviewer isn't really "owned" right now except by you, so you can somewhat set the target. This regression is saying basically that the events/sec regressed by 10% meaning you can record for 10% less time in frameviewer with this patch.

I think its on us (me, fadi, zhenw) to get to the bottom of  presets and a clear understanding of what presets we have, who the consumers are, and what their time and filesize targets are. Given that, we'll be able to then reason about regressions better.

Put another way, I think Vlad you can decide what to do here, since you're the closest thing we've got to a frameviewer owner right now. :) Is this worth it? My gut says yes but you've got to make the call as our GPU peep :D

Comment 8 by vmp...@chromium.org, Sep 16 2016

Hmm ok. I'll give another glance at how much of the information is actually useful (since the patch I have adds traces to all functions, some of which are really fast). However, this patch did help me identify an unexpectedly slow part in a function that was meant to be fast, so I think at least some of these traces are important.

Moreover, I think I would rather be more lenient with disabled-by-default traces since they (in my mind) are meant to record more detailed information even if it comes at a cost of recording for less time.
Project Member

Comment 9 by bugdroid1@chromium.org, Sep 16 2016

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

commit 38ff4c411c7982aa8dd5c36a59605ce45126723e
Author: vmpstr <vmpstr@chromium.org>
Date: Fri Sep 16 21:53:32 2016

cc: Remove some spammy traces from GpuImageDecodeController

This patch removes some traces that are not as useful, but happen
a lot.

R=danakj
BUG= 647363 
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Review-Url: https://codereview.chromium.org/2347123005
Cr-Commit-Position: refs/heads/master@{#419287}

[modify] https://crrev.com/38ff4c411c7982aa8dd5c36a59605ce45126723e/cc/tiles/gpu_image_decode_controller.cc

Is there any more that needs to be done here, or should this be marked as fixed?
Status: Fixed (was: Assigned)
I think we can close this. 

Sign in to add a comment