Issue metadata
Sign in to add a comment
|
6.4% regression in media_perftests at 528915:528941 |
||||||||||||||||||||||
Issue descriptionlooks like it recovered, but interested to see what it was.
,
Jan 18 2018
馃搷 Pinpoint job started. https://pinpoint-dot-chromeperf.appspot.com/job/14a659d8840000
,
Jan 18 2018
馃搷 Found a significant difference after 1 commit. https://pinpoint-dot-chromeperf.appspot.com/job/14a659d8840000 Reland "Make many base:: classes constexpr default constructible" By gab@chromium.org 路 Fri Jan 12 13:40:21 2018 chromium @ 2e4eab147b94a947ed4650234637b85802000b2b Understanding performance regressions: http://g.co/ChromePerformanceRegressions
,
Jan 19 2018
Hmmmm, this change is strictly a compiler change, no-op in logic (or lets compiler do more efficient optimization in theory perhaps).... suspicious it has anything to do with it...
,
Jan 22 2018
馃搷 Pinpoint job started. https://pinpoint-dot-chromeperf.appspot.com/job/148d6298840000
,
Jan 22 2018
馃搷 Pinpoint job started. https://pinpoint-dot-chromeperf.appspot.com/job/12ca04d8840000
,
Jan 22 2018
馃搷 Found a significant difference after 1 commit. https://pinpoint-dot-chromeperf.appspot.com/job/12ca04d8840000 Reland "Make many base:: classes constexpr default constructible" By gab@chromium.org 路 Fri Jan 12 13:40:21 2018 chromium @ 2e4eab147b94a947ed4650234637b85802000b2b Understanding performance regressions: http://g.co/ChromePerformanceRegressions
,
Jan 22 2018
馃搷 Pinpoint job started. https://pinpoint-dot-chromeperf.appspot.com/job/118f8ee8840000
,
Jan 22 2018
馃搷 Pinpoint job started. https://pinpoint-dot-chromeperf.appspot.com/job/149354b8840000
,
Jan 23 2018
馃搷 Found a significant difference after 1 commit. https://pinpoint-dot-chromeperf.appspot.com/job/149354b8840000 StatisticsRecorder doesn't need to be explicitly initialized. By fdegros@chromium.org 路 Mon Jan 08 01:18:30 2018 chromium @ e3226102363fa5a99183cfd891dd614e89896332 Understanding performance regressions: http://g.co/ChromePerformanceRegressions
,
Jan 23 2018
馃搷 Found a significant difference after 1 commit. https://pinpoint-dot-chromeperf.appspot.com/job/118f8ee8840000 StatisticsRecorder doesn't need to be explicitly initialized. By fdegros@chromium.org 路 Mon Jan 08 01:18:30 2018 chromium @ e3226102363fa5a99183cfd891dd614e89896332 Understanding performance regressions: http://g.co/ChromePerformanceRegressions
,
Jan 23 2018
馃搷 Found significant differences after each of 2 commits. https://pinpoint-dot-chromeperf.appspot.com/job/148d6298840000 Add traffic annotation auditor's executable path to md files. By rhalavati@google.com 路 Fri Jan 12 13:35:59 2018 chromium @ 2dfb924173c419f5c9cfeec74f8a87596f16b804 Reland "Make many base:: classes constexpr default constructible" By gab@chromium.org 路 Fri Jan 12 13:40:21 2018 chromium @ 2e4eab147b94a947ed4650234637b85802000b2b Understanding performance regressions: http://g.co/ChromePerformanceRegressions
,
Jan 23 2018
I honestly have no idea what's going on... If making some default constructors constexpr regressed any performance that's a clang bug IMO (CC thakis). But I'm not even convinced this is what's happening here... and since the graph jumped up (improved -- attributed to fdegros@) and then shortly after jumped down the same amount (regressed -- my CL is blamed). Since it's minimal both ways I'll go ahead and assume we shouldn't put more efforts into this?
,
Jan 23 2018
(in general, if you think something is a clang codegen bug, please post before/after asm)
,
Jan 23 2018
Good point, how would you go about doing that for a change in //base that affects the entire build (and which you don't know which .cc file was compiled differently because of it)?
,
Jan 23 2018
For perf regressions, we try to profile with and without change and then compare profiles to get an idea what got slower, and then look there.
,
Jan 23 2018
Hmmm, I see, but in this case it's not even clear if there is a regression (see OP) and I don't have a local repro... Maybe we ignore this one..?
,
Jan 23 2018
With my clang hat on: I don't have an opinion on what to do about the bug, just saying what we'd have to see to investigate a compiler bug. With my chrome hat on: 6.4% doesn't seem that small to me. It'd be surprising if this was due to the constexpr change. Since it's safe and easy to do, I'd try reverting each suspected change and see if it has an effect on the graph. If not, easy to reland. If so, there's an opportunity to learn something. (And if it _is_ due to constexpr against expectations, it'd be good to find that early, before we use that a ton all over the place.)
,
Jan 23 2018
Fair enough, revert under way : https://chromium-review.googlesource.com/c/chromium/src/+/881342
,
Jan 23 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/804a46fc4b631b3a6c25cc6ee782c1c5dac14aef commit 804a46fc4b631b3a6c25cc6ee782c1c5dac14aef Author: Gabriel Charette <gab@chromium.org> Date: Tue Jan 23 17:12:57 2018 Revert "Reland "Make many base:: classes constexpr default constructible"" This reverts commit 2e4eab147b94a947ed4650234637b85802000b2b. Reason for revert: Testing as culprit for crbug.com/803517 Original change's description: > Reland "Make many base:: classes constexpr default constructible" > > This is a reland of d74c23a26e423c3fb70b6216f5dcd2acb85f9157 > Original change's description: > > Make many base:: classes constexpr default constructible > > Same CL but extracted scoped_refptr changes to separate CL. > > > > > Bug: 800760 > > Change-Id: Ib713771347283825ff53a8517b38a8abccd9d1bb > > Reviewed-on: https://chromium-review.googlesource.com/860157 > > Commit-Queue: Gabriel Charette <gab@chromium.org> > > Reviewed-by: Daniel Cheng <dcheng@chromium.org> > > Cr-Commit-Position: refs/heads/master@{#528805} > > Bug: 800760 > Change-Id: Ia989d978f632075c21f11cd81687f52b9e2bd2ca > > TBR=dcheng@chromium.org > > Change-Id: Ia989d978f632075c21f11cd81687f52b9e2bd2ca > Reviewed-on: https://chromium-review.googlesource.com/863622 > Reviewed-by: Gabriel Charette <gab@chromium.org> > Commit-Queue: Gabriel Charette <gab@chromium.org> > Cr-Commit-Position: refs/heads/master@{#528934} TBR=dcheng@chromium.org,gab@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 800760, 803517 Change-Id: I1c2e3be23dbde603befffa0f7ec2153e0e5a1851 Reviewed-on: https://chromium-review.googlesource.com/881342 Reviewed-by: Gabriel Charette <gab@chromium.org> Commit-Queue: Gabriel Charette <gab@chromium.org> Cr-Commit-Position: refs/heads/master@{#531267} [modify] https://crrev.com/804a46fc4b631b3a6c25cc6ee782c1c5dac14aef/base/allocator/partition_allocator/spin_lock.cc [modify] https://crrev.com/804a46fc4b631b3a6c25cc6ee782c1c5dac14aef/base/allocator/partition_allocator/spin_lock.h [modify] https://crrev.com/804a46fc4b631b3a6c25cc6ee782c1c5dac14aef/base/android/scoped_java_ref.h [modify] https://crrev.com/804a46fc4b631b3a6c25cc6ee782c1c5dac14aef/base/containers/circular_deque.h [modify] https://crrev.com/804a46fc4b631b3a6c25cc6ee782c1c5dac14aef/base/containers/vector_buffer.h [modify] https://crrev.com/804a46fc4b631b3a6c25cc6ee782c1c5dac14aef/base/mac/scoped_dispatch_object.h [modify] https://crrev.com/804a46fc4b631b3a6c25cc6ee782c1c5dac14aef/base/mac/scoped_nsobject.h [modify] https://crrev.com/804a46fc4b631b3a6c25cc6ee782c1c5dac14aef/base/mac/scoped_typeref.h [modify] https://crrev.com/804a46fc4b631b3a6c25cc6ee782c1c5dac14aef/base/memory/ref_counted.h [modify] https://crrev.com/804a46fc4b631b3a6c25cc6ee782c1c5dac14aef/base/test/test_discardable_memory_allocator.cc [modify] https://crrev.com/804a46fc4b631b3a6c25cc6ee782c1c5dac14aef/base/test/test_discardable_memory_allocator.h [modify] https://crrev.com/804a46fc4b631b3a6c25cc6ee782c1c5dac14aef/base/threading/platform_thread.h
,
Jan 25 2018
Revert didn't result in getting this back at all. (also graph is regularly oscillating between these two values) Back to liberato@ (CC sullivan@ as liberato appears OOO?)
,
Jan 25 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/63fe7062928e18ef60169819ae5ab3934c0b22b0 commit 63fe7062928e18ef60169819ae5ab3934c0b22b0 Author: Gabriel Charette <gab@chromium.org> Date: Thu Jan 25 14:14:29 2018 Reland "Reland "Make many base:: classes constexpr default constructible"" This is a reland of 2e4eab147b94a947ed4650234637b85802000b2b. Doesn't appear to be at fault for crbug.com/803517 as suspected. Original change's description: > Reland "Make many base:: classes constexpr default constructible" > > This is a reland of d74c23a26e423c3fb70b6216f5dcd2acb85f9157 > Original change's description: > > Make many base:: classes constexpr default constructible > > Same CL but extracted scoped_refptr changes to separate CL. > > > > > Bug: 800760 > > Change-Id: Ib713771347283825ff53a8517b38a8abccd9d1bb > > Reviewed-on: https://chromium-review.googlesource.com/860157 > > Commit-Queue: Gabriel Charette <gab@chromium.org> > > Reviewed-by: Daniel Cheng <dcheng@chromium.org> > > Cr-Commit-Position: refs/heads/master@{#528805} > > Bug: 800760 > Change-Id: Ia989d978f632075c21f11cd81687f52b9e2bd2ca > > TBR=dcheng@chromium.org > > Change-Id: Ia989d978f632075c21f11cd81687f52b9e2bd2ca > Reviewed-on: https://chromium-review.googlesource.com/863622 > Reviewed-by: Gabriel Charette <gab@chromium.org> > Commit-Queue: Gabriel Charette <gab@chromium.org> > Cr-Commit-Position: refs/heads/master@{#528934} TBR=dcheng@chromium.org Bug: 800760, 803517 Change-Id: Ic987b883e6ccacaa0b2e26b55d875cf4b484a5cc Reviewed-on: https://chromium-review.googlesource.com/886323 Commit-Queue: Gabriel Charette <gab@chromium.org> Reviewed-by: Gabriel Charette <gab@chromium.org> Cr-Commit-Position: refs/heads/master@{#531890} [modify] https://crrev.com/63fe7062928e18ef60169819ae5ab3934c0b22b0/base/allocator/partition_allocator/spin_lock.cc [modify] https://crrev.com/63fe7062928e18ef60169819ae5ab3934c0b22b0/base/allocator/partition_allocator/spin_lock.h [modify] https://crrev.com/63fe7062928e18ef60169819ae5ab3934c0b22b0/base/android/scoped_java_ref.h [modify] https://crrev.com/63fe7062928e18ef60169819ae5ab3934c0b22b0/base/containers/circular_deque.h [modify] https://crrev.com/63fe7062928e18ef60169819ae5ab3934c0b22b0/base/containers/vector_buffer.h [modify] https://crrev.com/63fe7062928e18ef60169819ae5ab3934c0b22b0/base/mac/scoped_dispatch_object.h [modify] https://crrev.com/63fe7062928e18ef60169819ae5ab3934c0b22b0/base/mac/scoped_nsobject.h [modify] https://crrev.com/63fe7062928e18ef60169819ae5ab3934c0b22b0/base/mac/scoped_typeref.h [modify] https://crrev.com/63fe7062928e18ef60169819ae5ab3934c0b22b0/base/memory/ref_counted.h [modify] https://crrev.com/63fe7062928e18ef60169819ae5ab3934c0b22b0/base/test/test_discardable_memory_allocator.cc [modify] https://crrev.com/63fe7062928e18ef60169819ae5ab3934c0b22b0/base/test/test_discardable_memory_allocator.h [modify] https://crrev.com/63fe7062928e18ef60169819ae5ab3934c0b22b0/base/threading/platform_thread.h
,
Jan 25 2018
Thanks for the clear explanation, gab, and sorry for the noise. Caleb, it looks like this test is bimodal, which causes false alerts and bisects that can point to random CLs. Can you help triage while liberato is out?
,
Jan 25 2018
Assigning to dalecurtis@ to triage. This is supposed to be the optimized and aligned version, which shouldn't be as flaky... Is it the result of compilation that produces different results after seemingly unrelated changes, i.e. once it is built it will always have the same value but it is built in two different ways depending on seemingly randomness? Or is it that each run can have a different result? If the former, I worry that this type of compilation is incompatible with micro-benchmarks like media_perftests, and could even be a problem for larger benchmarks like the system health benchmarks (once they remove enough noise in those to get down to this level). If you read the code for media_perftest's sinc_resampler_convolve, you can see that all it does is run an operation in a loop to figure out how many times it can run it in a millisecond: runs/ms. So I'm wondering what video stack team could do to fix it: it seems like a lower-level problem than our code.
,
Jan 25 2018
One note in case this is the result of compilation: The bisect uses prebuilt binaries, so that could cause us to get consistent bisect results even though there is no actual performance regression.
,
Jan 25 2018
Probably we should just turn off alerting for these tests since they seem sensitive to differences in binary composition / distant changes. The data is good to have on a historical basis and they're quick tests, so I think they're worth keeping overall though for lookup when folks do make changes here. In any case, the total regression is not much since this code runs 50-100 times a second and values being put up are in the hundreds of thousands so closing as WontFix.
,
Jan 25 2018
Caleb, can you help with tuning the alerting? You could consider making it less sensitive instead of turning it off altogether. Follow up offline if you need any help!
,
Jan 25 2018
Filed issue 806009 . |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by 42576172...@developer.gserviceaccount.com
, Jan 18 2018