Closed Bug 1415465 Opened 3 years ago Closed 3 years ago
.48% Strings Perf Strip Chars Whitespace (osx-10-10) regression on push 17decbfb072038b166a5f658e60483ebb56742e0 (Sat Nov 4 2017)
We have detected a platform microbenchmarks regression from push: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=17decbfb072038b166a5f658e60483ebb56742e0 As author of one of the patches included in that push, we need your help to address this regression. Regressions: 12% Strings PerfStripCharsWhitespace osx-10-10 opt 359,108.21 -> 403,925.83 Improvements: 8% TestStandardURL Perf osx-10-10 opt 55,994.12 -> 51,309.83 5% Stylo Servo_DeclarationBlock_GetPropertyById_Bench osx-10-10 opt 213,421.29 -> 202,141.08 5% TestStandardURL NormalizePerf osx-10-10 opt 52,870.12 -> 50,174.83 4% Stylo Gecko_nsCSSParser_ParseSheet_Bench osx-10-10 opt 69,573.08 -> 66,727.00 You can find links to graphs and comparison views for each of the above tests at: https://treeherder.mozilla.org/perf.html#/alerts?id=10347 On the page above you can see an alert for each affected platform as well as a link to a graph showing the history of scores for this test. There is also a link to a treeherder page showing the jobs in a pushlog format. To learn more about the regressing test(s), please see: https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Automated_Performance_Testing_and_Sheriffing/Platform_Microbenchmarks
Component: Untriaged → XPCOM
Product: Firefox → Core
:cpeterson Bug 1406398 made the Strings PerfStripCharsWhitespace bimodal. As the minimal values were lower than the previous ones, we gained a perf improvement. Bug 1412048 made the results uni-modal again. Though I prefer the new graph to remain uni-modal, I want to make sure this isn't a serious regression which sneaked in. :jandem and :cpeterson, can you please explain how your bugs made the test bi-modal, then uni-modal? :jandem, I'm asking you because I cannot find André Bargull's email address and you reviewed his work.
(In reply to Ionuț Goldan [:igoldan], Performance Sheriffing from comment #1) > Bug 1412048 made the results uni-modal again. Though I prefer the new graph > to remain uni-modal, I want to make sure this isn't a serious regression > which sneaked in. Bug 1412048 replaced some calls to NS_RUNTIMEABORT with MOZ_CRASH_UNSAFE_PRINTF: https://hg.mozilla.org/integration/mozilla-inbound/rev/e85f59ea455d None of those functions are actually called during these test (otherwise the test would have crashed :). I suspect this PerfStripCharsWhitespace regression was caused because MOZ_CRASH_UNSAFE_PRINTF added more out-of-line code to the CHECK_STRING_BUFFER_CANARY macro than NS_RUNTIMEABORT did. Fortunately, I don't think we need to do anything for this regression. CHECK_STRING_BUFFER_CANARY is temporary diagnostic code that was added in bug 1410276 and was just backed yesterday: https://hg.mozilla.org/integration/mozilla-inbound/rev/23f86a1ac424 When CHECK_STRING_BUFFER_CANARY first landed, it caused Gecko_nsCSSParser_ParseSheet_Bench / Servo_DeclarationBlock_GetPropertyById_Bench regression bug 1412208. So the regressions in this bug and bug 1412208 should go away today.
(In reply to Ionuț Goldan [:igoldan], Performance Sheriffing from comment #1) > :cpeterson Bug 1406398 made the Strings PerfStripCharsWhitespace bimodal. As > the minimal values were lower than the previous ones, we gained a perf > improvement. That bug seems unrelated. It's a trivial change and, more importantly, PerfStripCharsWhitespace is a C++ test and that bug affects some code in the JS engine that shouldn't be exercised here.
Bug 1412208 got fixed, indeed. This PerfStripCharsWhitespace regression however, didn't. Should we look further into this or rather accept this?
(In reply to Ionuț Goldan [:igoldan], Performance Sheriffing from comment #4) > Bug 1412208 got fixed, indeed. This PerfStripCharsWhitespace regression > however, didn't. Should we look further into this or rather accept this? I don't know why my changes in bug 1412048 would still affect PerfStripCharsWhitespace after bug 1412208 was fixed. I can try testing locally or backing out my change.
Interesting. I am able to reproduce a performance difference with and without my patch from bug 1412048 on my MacBook Pro, but my patch improves PerfStripCharsWhitespace performance about 6%! With my MOZ_CRASH_UNSAFE_PRINTF patch, the test takes about 1774 - 1779 ms. Without my patch, the test takes about 1888 - 1911 ms. I'm surprised that my change to a few IPC functions that are never called would speed up string operations in a C++ gtest. I don't know why Jan's bug 1406398 made PerfStripCharsWhitespace bimodal or why my MOZ_CRASH_UNSAFE_PRINTF patch made it unimodal again, but I don't know what else to do. The current unimodal results are the same as before the bimodal period, so at least performance is not worse than before: https://treeherder.mozilla.org/perf.html#/graphs?timerange=2592000&series=mozilla-inbound,1487667,1,6
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.