Closed
Bug 1415465
Opened 7 years ago
Closed 7 years ago
12.48% Strings PerfStripCharsWhitespace (osx-10-10) regression on push 17decbfb072038b166a5f658e60483ebb56742e0 (Sat Nov 4 2017)
Categories
(Core :: XPCOM, defect)
Tracking
()
RESOLVED
WONTFIX
Tracking | Status | |
---|---|---|
firefox58 | --- | affected |
People
(Reporter: igoldan, Unassigned)
References
Details
(Keywords: perf, regression)
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
Reporter | ||
Updated•7 years ago
|
Component: Untriaged → XPCOM
Product: Firefox → Core
Reporter | ||
Comment 1•7 years ago
|
||
: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.
Flags: needinfo?(jdemooij)
Flags: needinfo?(cpeterson)
Comment 2•7 years ago
|
||
(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.
Blocks: 1410276
Flags: needinfo?(cpeterson)
Comment 3•7 years ago
|
||
(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.
Updated•7 years ago
|
Flags: needinfo?(jdemooij)
Updated•7 years ago
|
status-firefox58:
--- → affected
Reporter | ||
Comment 4•7 years ago
|
||
Bug 1412208 got fixed, indeed. This PerfStripCharsWhitespace regression however, didn't. Should we look further into this or rather accept this?
Flags: needinfo?(cpeterson)
Comment 5•7 years ago
|
||
(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.
Flags: needinfo?(cpeterson)
Comment 6•7 years ago
|
||
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: 7 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•