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

Issue 751973 link

Starred by 1 user

Issue metadata

Status: WontFix
Owner:
Closed: Aug 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

19.3% regression in performance_browser_tests at 489680:489795

Project Member Reported by alexclarke@chromium.org, Aug 3 2017

Issue description

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

(For debugging:) Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?sid=3759d2923d07c3228cb5e4ac043beb1072def8d76bd86c9bf1cf09a510b0396c


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

chromium-rel-win7-x64-dual
Cc: gab@chromium.org
Owner: gab@chromium.org

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

Hi gab@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 : Gabriel Charette
  Commit : aa061e550891fab4b55e75d85955e27eed25bf94
  Date   : Wed Jul 26 20:18:46 2017
  Subject: SetupMetrics() in PreCreateThreads() to fix data race in ScopedTaskTracker initialization.

Bisect Details
  Configuration: win_x64_perf_bisect
  Benchmark    : performance_browser_tests
  Metric       : TabCapturePerformance_comp_gpu_webrtc/Capture
  Change       : 26.42% | 19.8044923333 -> 25.0367108333

Revision             Result                  N
chromium@489679      19.8045 +- 1.54002      6      good
chromium@489737      19.5597 +- 1.0822       6      good
chromium@489738      20.1845 +- 2.29619      6      good
chromium@489739      24.3906 +- 3.4969       6      bad       <--
chromium@489741      25.4363 +- 1.7512       6      bad
chromium@489745      25.0487 +- 2.46549      6      bad
chromium@489752      25.4734 +- 2.15376      6      bad
chromium@489766      24.3128 +- 2.61744      6      bad
chromium@489795      25.0367 +- 3.19418      6      bad

To Run This Test
  .\src\out\Release_x64\performance_browser_tests.exe --test-launcher-print-test-stdio=always --enable-gpu

More information on addressing performance regressions:
  http://g.co/ChromePerformanceRegressions

Debug information about this bisect:
  https://chromeperf.appspot.com/buildbucket_job_status/8972297826296129840


For feedback, file a bug with component Speed>Bisection

Comment 4 by gab@chromium.org, Aug 3 2017

Cc: asvitk...@chromium.org
Components: Internals>Metrics
Alexei, any idea why moving SetupMetrics() earlier would have regressed performance_browser_tests TabCapturePerformance_comp_gpu_webrtc/Capture ?
Cc: hubbe@chromium.org justinlin@chromium.org m...@chromium.org
I do not. +cc owners of the test in question.

Looking at the benchmark code:

https://cs.corp.google.com/clankium/src/chrome/browser/extensions/api/tab_capture/tab_capture_performancetest.cc?q=TabCapturePerformance&dr=CSs&l=58

I think it's trying to find durations of "Capture" trace events, which are here:

https://cs.chromium.org/chromium/src/media/capture/content/thread_safe_capture_oracle.cc?rcl=f768b8dab842bdde2485d0c65c070ddbf9dff3ae&l=134

I don't see how that would be affected by metrics service. Obviously the CL in question moves around MetricsService::InitializeMetricsRecordingState() which does do some work - but I was trying to look to see if it records a timestamp somewhere that could be used by the benchmark, but didn't see anything like that. It does post tasks and such - and I wonder if it could interact with tab capture tasks timing indirectly? Interestingly, the tab capture perf tests has a constant about events to trim:


// Number of events to trim from the begining and end. These events don't
// contribute anything toward stable measurements: A brief moment of startup
// "jank" is acceptable, and shutdown may result in missing events (since
// render widget draws may stop before capture stops).
constexpr size_t kTrimEvents = 24; 

I wonder if the startup jank it's referring to could be at play here - i.e. moving things around at startup could have wiggled things around...




For reference, here's the sampling profiler delta between the two versions where that change was landed:

https://uma.googleplex.com/callstacks?sid=0b0c82e3d16063d8e9228fb0eb3162f7

Comment 7 by m...@chromium.org, Aug 7 2017

Status: WontFix (was: Untriaged)
Hmm...Looking at the graphs, I wonder if this a fluke. FWIW, there's this evidence:

1. All other chromium-rel-win7-x64-dual/performance_browser_tests/*/Capture tests seem unaffected.

2. There was an "unexplained improvement" at r485508, whereby at r489739 simply brought it back to prior levels.

3. Post r489739, performance slightly improved and otherwise seems stable.

I also don't see how moving metrics initialization could have an impact on this perf test. Even any unintentional startup latency should not have had an effect since these tests ignore the tracing events during the few seconds of the run to abate that exact issue.

So, I think it's safe to WontFix this.

Sign in to add a comment