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

Issue 641966 link

Starred by 2 users

Issue metadata

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



Sign in to add a comment

82.5% regression in webrtc_perf_tests at 13946:13946 due to changed optimizations.

Project Member Reported by hlundin@chromium.org, Aug 29 2016

Issue description

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

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


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

webrtc-win-large-tests
Cc: ivoc@chromium.org
Labels: -M-52 M-55
Owner: kjellander@chromium.org
kjellander: this is caused by "MB: Flip Windows bots to GN by default" (https://codereview.webrtc.org/2277253002). Is this expected?
Cc: ehmaldonado@chromium.org
Labels: OS-Windows
Certainly not expected. Edward and I were just about to start auditing the differences between the GYP and GN build. Seems like we'll start by looking at this target tomorrow!
This regression might be caused because GYP is compiled with /O2 while GN is compiled with /O1

It seems like the default in GYP is O2:
https://cs.chromium.org/chromium/src/build/common.gypi?l=2482

But it's O1 in GN: https://cs.chromium.org/chromium/src/build/config/compiler/BUILD.gn?l=1345

Although there is also a "optimize_max" compile config in GN:
https://cs.chromium.org/chromium/src/build/config/compiler/BUILD.gn?l=1398

The differences in the compile flags are:
   In gyp, but not in GN:
     /GF
     /GS
     /O2
     /fp:precise
     /we4389
   In GN, but not in gyp:
     /O1
     /Oi

Comment 5 Deleted

Cc: -ehmaldonado@chromium.org kjellander@chromium.org
Owner: ehmaldonado@chromium.org
Status: Started (was: Assigned)
I'm going to let you own this, since you're already looking at it.
Hmm, it doesn't seem like Edwards CL https://codereview.webrtc.org/2291253003 helped. The points in the graph with ID 14001 is unchanged. I verified in https://build.chromium.org/p/client.webrtc/builders/Win32%20Release%20%5Blarge%20tests%5D/builds/8658/steps/compile/logs/stdio that the relevant object are actually recompiled. I'll force a clobber build on the bot just to be sure.
Project Member

Comment 8 by bugdroid1@chromium.org, Sep 5 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/external/webrtc.git/+/59af8b77145b8ff33a610128d339f02f950d3e7a

commit 59af8b77145b8ff33a610128d339f02f950d3e7a
Author: ehmaldonado <ehmaldonado@webrtc.org>
Date: Mon Sep 05 09:48:54 2016

GN Templates: Use the optimize_max compiler config.

Add "//build/config/compiler:optimize_max" to rtc_add_configs and
"//build/config/compiler:default_optimization" to rtc_remove_configs.

This is the default optimization in GYP, and might help explain a 82.5%
regression in webrtc_perf_tests at 13946:13946

BUG= chromium:641966 
NOTRY=True

Review-Url: https://codereview.webrtc.org/2307283002
Cr-Commit-Position: refs/heads/master@{#14067}

[modify] https://crrev.com/59af8b77145b8ff33a610128d339f02f950d3e7a/webrtc/BUILD.gn
[modify] https://crrev.com/59af8b77145b8ff33a610128d339f02f950d3e7a/webrtc/build/webrtc.gni
[modify] https://crrev.com/59af8b77145b8ff33a610128d339f02f950d3e7a/webrtc/modules/audio_coding/BUILD.gn

Status: Fixed (was: Started)
Wohoo, graphs are restored with the commit in #8 above!
Cc: hlundin@chromium.org
Nico asked in https://codereview.webrtc.org/2307283002/ if it was necessary as they saw a size regression in Chrome due to our fix.

+hlundin regarding if this regression is serious or something we can just discard referencing the changed optimization level.

Cc: thakis@chromium.org
Summary: 82.5% regression in webrtc_perf_tests at 13946:13946 due to changed optimizations. (was: 82.5% regression in webrtc_perf_tests at 13946:13946)
Another question in https://codereview.webrtc.org/2307283002/: Does NetEq test a code path that's hot in chrome as well?
hlundin: can you provide a detailed answer?
This test runs NetEq stand-alone at maximum speed (faster than real-time).

NetEq usually accounts for just a percent or so of the total WebRTC load. Video encoding, decoding and processing are usually much larger. So even an 80% regression for NetEq probably only translates to a few percents in total for an HD Video WebRTC call.

But the question is how many other components are affected by this. This test looks only at NetEq.

So to me it sounds like increasing chrome's size quite a bit for this isn't worth it. Is that incorrect? (It looks like that patch is still in, so I guess someone disagrees? Why?)
I'm reverting the optimization change for Win Release in https://codereview.webrtc.org/2334593002/ so we're going to see this test regress again.

I'll inform our perf sheriffs so they can connect the new alerts to this bug.
Project Member

Comment 15 by bugdroid1@chromium.org, Sep 12 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/external/webrtc.git/+/11d57668190edb1f4b8208500da3ab33f638dee3

commit 11d57668190edb1f4b8208500da3ab33f638dee3
Author: kjellander <kjellander@webrtc.org>
Date: Mon Sep 12 06:29:36 2016

GN: Revert to default compiler optimizations for Win Release.

Revert back to the GN/Chromium default optimization levels what
was changed in https://codereview.webrtc.org/2307283002/ and
https://codereview.webrtc.org/2305403002/ due to a performance
regression. Those changes caused a size regression in Chromium,
which is why this is changed back.

BUG= chromium:641966 
NOTRY=True
TBR=ehmaldonado@webrtc.org, henrik.lundin@webrtc.org

Review-Url: https://codereview.webrtc.org/2334593002
Cr-Commit-Position: refs/heads/master@{#14178}

[modify] https://crrev.com/11d57668190edb1f4b8208500da3ab33f638dee3/webrtc/build/webrtc.gni

 Issue 646346  has been merged into this issue.

Sign in to add a comment