1415465 - 12.48% Strings PerfStripCharsWhitespace (osx-10-10) regression on push 17decbfb072038b166a5f658e60483ebb56742e0 (Sat Nov 4 2017)

Status: RESOLVED WONTFIX

(Core :: XPCOM, defect)

Description

[Ionuț Goldan [:igoldan]]

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

Comment 1

[Ionuț Goldan [:igoldan]]

: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.

Comment 2

[Chris Peterson [:cpeterson]]

(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.

Comment 3

[Jan de Mooij [:jandem]]

(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.

Comment 4

[Ionuț Goldan [:igoldan]]

Bug 1412208 got fixed, indeed. This PerfStripCharsWhitespace regression however, didn't. Should we look further into this or rather accept this?

Comment 5

[Chris Peterson [:cpeterson]]

(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.

Comment 6

[Chris Peterson [:cpeterson]]

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