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

Issue 736837 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Jun 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

373.7% regression in browser_tests/video_tx_VP9/goog_rtt on chromium-webrtc-trunk-tot-rel-win8 at 482167:482168

Project Member Reported by niklase@google.com, Jun 26 2017

Issue description

Cc: niklase@chromium.org
Components: Blink>WebRTC
Owner: kjellander@chromium.org
Status: Assigned (was: Unconfirmed)
Status: Started (was: Assigned)
I think I've found the culprit:
https://chromium.googlesource.com/chromium/src/+/d5b40133c57d80b9368a832ee8ad497d34aab392

It makes the behavior of the --test-launcher-jobs=1 flag, which we use to make the tests not run in parallel (since we use them for perf tests).
By comparing https://build.chromium.org/p/chromium.webrtc/builders/Linux%20Tester/builds/30230
and
https://build.chromium.org/p/chromium.webrtc/builders/Linux%20Tester/builds/30231
you can see that the execution time of browser_tests goes down from 48 to 14 minutes, due to the parallel execution.

I'll check if we can easily just update our buildbot recipes.
Reverting it in https://chromium-review.googlesource.com/c/549295
Don't have time to look more into this today.

Comment 4 by most...@opera.com, Jun 26 2017

Cc: most...@opera.com
Project Member

Comment 5 by bugdroid1@chromium.org, Jun 26 2017

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

commit ec515d037019e5a1ceace133a5bdfb005cb5caf5
Author: Henrik Kjellander <kjellander@chromium.org>
Date: Mon Jun 26 22:30:52 2017

Revert "base::LaunchUnitTestsSerially should not be overridable"

This reverts commit d5b40133c57d80b9368a832ee8ad497d34aab392.

Reason for revert: Breaks all WebRTC browser_tests that runs on real audio and video devices (tests marked as MANUAL), since they rely on the behavior of --test-launcher-jobs=1. With this CL those tests started running in parallel, which doesn't work since each test needs exclusive access to the webcam. This also made all the perf measurements become very unstable.
See  crbug.com/736837  for examples.

I'll be happy to update our bots to use another flag, but I need to revert this now to bring our perf bots back into a stable state asap. Please reach out to me before relanding this.

We run our tests with --run-manual --ui-test-action-max-timeout=350000 --test-launcher-jobs=1 --test-launcher-bot-mode --test-launcher-print-test-stdio=always
today. I notice there's also --single-process-tests available, but I'm not convinced that means they don't run in parallel, which is why I'm reverting this to be sure for now.

Original change's description:
> base::LaunchUnitTestsSerially should not be overridable
> 
> base::LaunchUnitTestsSerially should ignore the --test-launcher-jobs
> command line switch, and never use parallel jobs.
> 
> BUG= 735587 
> 
> Change-Id: I7e455a805b898f12e7adc91a41feb9627e008964
> Reviewed-on: https://chromium-review.googlesource.com/543344
> Commit-Queue: Mostyn Bramley-Moore <mostynb@opera.com>
> Reviewed-by: Paweł Hajdan Jr. <phajdan.jr@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#481990}

TBR=phajdan.jr@chromium.org,mostynb@opera.com

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  735587 ,  736837 
Change-Id: Ief9b133c9daab91dec6bcb7a920f12211d03e622
Reviewed-on: https://chromium-review.googlesource.com/549295
Reviewed-by: Henrik Kjellander <kjellander@chromium.org>
Commit-Queue: Henrik Kjellander <kjellander@chromium.org>
Cr-Commit-Position: refs/heads/master@{#482438}
[modify] https://crrev.com/ec515d037019e5a1ceace133a5bdfb005cb5caf5/base/test/launcher/test_launcher.cc
[modify] https://crrev.com/ec515d037019e5a1ceace133a5bdfb005cb5caf5/base/test/launcher/test_launcher.h
[modify] https://crrev.com/ec515d037019e5a1ceace133a5bdfb005cb5caf5/base/test/launcher/unit_test_launcher.cc
[modify] https://crrev.com/ec515d037019e5a1ceace133a5bdfb005cb5caf5/base/test/launcher/unit_test_launcher.h

Status: Fixed (was: Started)
I verified the graphs are back to normal now.
Project Member

Comment 7 by bugdroid1@chromium.org, Jul 1 2017

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

commit d5588cf4ffb5a932dcbfd87cd7761d390c790ea9
Author: Mostyn Bramley-Moore <mostynb@opera.com>
Date: Sat Jul 01 12:57:18 2017

(reland) base::LaunchUnitTestsSerially should not be overridable by cmdline

(reland) base::LaunchUnitTestsSerially should ignore the --test-launcher-jobs
command line switch, and never use parallel jobs.  To achieve this, change
callers of base::TestLauncher to pass the requested number of parallel test
jobs, instead of the default number of parallel test jobs.

And while we're at it, let's remove TestLauncherDelegate's
AdjustDefaultParallelJobs method, which no longer has any users.

BUG= 735587 , 736837 
TBR=rdevlin.cronin@chromium.org

Change-Id: I4085fb21c1dce467527210407e3913ff3b5e3bc6
Reviewed-on: https://chromium-review.googlesource.com/549342
Commit-Queue: Mostyn Bramley-Moore <mostynb@opera.com>
Reviewed-by: Henrik Kjellander <kjellander@chromium.org>
Reviewed-by: Alok Priyadarshi <alokp@chromium.org>
Reviewed-by: Paweł Hajdan Jr. <phajdan.jr@chromium.org>
Reviewed-by: Alex Clarke <alexclarke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#483909}
[modify] https://crrev.com/d5588cf4ffb5a932dcbfd87cd7761d390c790ea9/base/test/launcher/test_launcher.cc
[modify] https://crrev.com/d5588cf4ffb5a932dcbfd87cd7761d390c790ea9/base/test/launcher/test_launcher.h
[modify] https://crrev.com/d5588cf4ffb5a932dcbfd87cd7761d390c790ea9/base/test/launcher/unit_test_launcher.cc
[modify] https://crrev.com/d5588cf4ffb5a932dcbfd87cd7761d390c790ea9/base/test/launcher/unit_test_launcher.h
[modify] https://crrev.com/d5588cf4ffb5a932dcbfd87cd7761d390c790ea9/chrome/test/base/browser_perf_tests_main.cc
[modify] https://crrev.com/d5588cf4ffb5a932dcbfd87cd7761d390c790ea9/chrome/test/base/browser_tests_main.cc
[modify] https://crrev.com/d5588cf4ffb5a932dcbfd87cd7761d390c790ea9/chrome/test/base/browser_tests_main_chromeos.cc
[modify] https://crrev.com/d5588cf4ffb5a932dcbfd87cd7761d390c790ea9/chrome/test/base/chrome_test_launcher.cc
[modify] https://crrev.com/d5588cf4ffb5a932dcbfd87cd7761d390c790ea9/chrome/test/base/chrome_test_launcher.h
[modify] https://crrev.com/d5588cf4ffb5a932dcbfd87cd7761d390c790ea9/chrome/test/base/interactive_ui_tests_main.cc
[modify] https://crrev.com/d5588cf4ffb5a932dcbfd87cd7761d390c790ea9/chrome/test/base/mash_browser_tests_main.cc
[modify] https://crrev.com/d5588cf4ffb5a932dcbfd87cd7761d390c790ea9/chromecast/app/cast_test_launcher.cc
[modify] https://crrev.com/d5588cf4ffb5a932dcbfd87cd7761d390c790ea9/content/public/test/test_launcher.cc
[modify] https://crrev.com/d5588cf4ffb5a932dcbfd87cd7761d390c790ea9/content/public/test/test_launcher.h
[modify] https://crrev.com/d5588cf4ffb5a932dcbfd87cd7761d390c790ea9/content/test/content_test_launcher.cc
[modify] https://crrev.com/d5588cf4ffb5a932dcbfd87cd7761d390c790ea9/extensions/shell/test/shell_tests_main.cc
[modify] https://crrev.com/d5588cf4ffb5a932dcbfd87cd7761d390c790ea9/headless/test/headless_test_launcher.cc

Sign in to add a comment