Issue metadata
Sign in to add a comment
|
10.4% regression in tracing.tracing_with_debug_overhead at 417992:418036 |
||||||||||||||||||||
Issue descriptionI believe that this is https://codereview.chromium.org/2328523004 this metric is measuring the spam-level of tracing
,
Sep 15 2016
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/9001428349052411728
,
Sep 15 2016
vmpstr, I fear this might be really https://codereview.chromium.org/2328523004
,
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.
,
Sep 15 2016
=== 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!
,
Sep 15 2016
Good question, +fmeawad +nduca
,
Sep 16 2016
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
,
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.
,
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
,
Oct 6 2016
Is there any more that needs to be done here, or should this be marked as fixed?
,
Oct 6 2016
I think we can close this. |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by primiano@chromium.org
, Sep 15 2016