Issue metadata
Sign in to add a comment
|
4.6% regression in webrtc_perf_tests at 14345:14347 |
||||||||||||||||||||
Issue descriptionSee graph below.
,
Sep 29 2016
Can you please have a look whether https://codereview.webrtc.org/2342443005 is the cause for this change and verify if it is expected.
,
Oct 4 2016
I've been unable to reproduce this locally - although I've only tried on my workstation, not on an actual device. I'm going to let the change I made that's related to issue 651426 land first and see if the numbers improve here as well.
,
Oct 5 2016
There's no clear improvement since 14495 landed (which addresses issue 651426 ). I'll take a proper look at this one again.
,
Oct 6 2016
I've run through revisions 14343 through 14351 on a Nexus 4 (the closest to a Nexus 7.2 I could get a hold of) and now believe the regression is due to added DCHECKs in the code. The runtime averaged over five runs for each revision is as follows (revisions 14349 and 14350 didn't introduce any changes to webrtc_perf_tests):
dcheck dcheck
rev. | on | off
14343 | 18962 | 16437
14344 | 18969 | 16376
14345 | 18982 | 16338
14346 | 18984 | 16238
14347 | 20014 | 16164 <-- my CL
14348 | 19745 | 15993
14351 | 19776 | 15901
Strangely enough, it looks like release-build performance actually increased with my CL, rather than decreased. I expect the perf tests (like our trybots) build with dcheck_always_on=true, which would corroborate my findings. Who would know if that is the case?
,
Oct 7 2016
+phoglund Are the binaries built for perf tests using dcheck_always_on=true, and should they? (See previous comment)
,
Oct 31 2016
Yes: https://uberchromegw.corp.google.com/i/client.webrtc.perf/builders/Android32%20Builder/builds/224/steps/generate_build_files/logs/stdio Should they? Not sure. I think we've discussed this at length before, but I don't remember what we came up with.
,
Oct 31 2016
We've discussed this in the past until recently we've been using the "large tests" bots (which build Release builds) as a platform for both correctness and performance tests. This is not ideal and Chromium has a clear separation between the two. Having this, the benefit of having DCHECKs enabled for the non-perf tests was prioritized over disabling them for the perf tests (doing an additional compilation was another option but it wasn't considered). Now, however, we've created a separate perf waterfall: client.webrtc.perf and we recently moved the Android perf bots in there when we flipped to Swarming. We're going to do the same for Desktop as soon we get 3 new machines allocated: bug 657572. So right now, we can remove dcheck_always_on=true for these Android bots, but not for the desktop platforms. I filed https://bugs.chromium.org/p/webrtc/issues/detail?id=6635 to track that work.
,
Oct 31 2016
Greatness! I'm closing this bug as WontFix, since as far as I can tell, the measurements didn't highlight a proper regression (i.e. only due to DCHECKs). The future changes to perf bots, turning DCHECKs off, will likely invalidate these measurements anyways, both before and after this regression. |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by asapersson@chromium.org
, Sep 29 2016