Closed Bug 1596172 Opened 1 year ago Closed 1 year ago

Add perf-reftest-singletons to PGO training

Categories

(Firefox Build System :: Toolchains, enhancement)

enhancement
Not set
normal

Tracking

(firefox72 fixed)

RESOLVED FIXED
mozilla72
Tracking Status
firefox72 --- fixed

People

(Reporter: dmajor, Assigned: dmajor)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

Recent changes in bug 1592981 caused a regression in the perf-reftest-singletons, mostly because nsStringBuffer::Release() stopped getting inlined. Fixing that is not quite as simple as tossing a MOZ_ALWAYS_INLINE on there: since Release() is a class method, it has different semantics about inline, so you get linker errors with the naive approach.

I tried reworking the function in various ways, but the compiler still wouldn't give me the code I wanted. Knowing that profile data can override bug 1592981's settings, I propose adding the test suite to PGO.

Also I'm hoping that this will increase the stability of this test suite as well. The singletons have been particularly fussy about toolchains updates in the past, and I'd prefer not to keep chasing after fluctuations in them.

This ought to overcome a regression from 1592981 as well as make this test suite less noisy about toolchain updates in the future.

While here, tidy up the PGO list: sort alphabetically, stop using a hardcoded test index for the Speedometer time extension, and stop using js-input/ for the virtual mappings since js-input/ is an actual directory in our repo.

Blocks: 1543849
Blocks: 1596551
Pushed by dmajor@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0ee409403100
Add perf-reftest-singletons to PGO training r=firefox-build-system-reviewers,mshal
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla72

== Change summary for alert #23969 (as of Fri, 15 Nov 2019 10:15:28 GMT) ==

Improvements:

24% perf_reftest_singletons external-string-pass.html linux64-shippable-qr opt e10s stylo 994.16 -> 757.21
24% perf_reftest_singletons external-string-pass.html linux64-shippable-qr opt e10s stylo 990.38 -> 755.36
24% perf_reftest_singletons external-string-pass.html linux64-shippable opt e10s stylo 958.72 -> 732.86
8% perf_reftest_singletons tiny-traversal-singleton.html linux64-shippable opt e10s stylo 836.80 -> 772.42
7% perf_reftest_singletons id-getter-6.html linux64-shippable opt e10s stylo 454.92 -> 421.51
7% perf_reftest_singletons id-getter-4.html linux64-shippable opt e10s stylo 453.94 -> 421.02
7% perf_reftest_singletons id-getter-5.html linux64-shippable opt e10s stylo 454.01 -> 421.21
7% perf_reftest_singletons id-getter-4.html linux64-shippable-qr opt e10s stylo 477.33 -> 444.93
7% perf_reftest_singletons id-getter-3.html linux64-shippable-qr opt e10s stylo 476.36 -> 444.14
7% perf_reftest_singletons id-getter-3.html linux64-shippable opt e10s stylo 453.65 -> 423.45
7% perf_reftest_singletons id-getter-6.html linux64-shippable-qr opt e10s stylo 475.80 -> 444.59
7% perf_reftest_singletons scrollbar-styles-1.html linux64-shippable opt e10s stylo 494.23 -> 462.10
6% perf_reftest_singletons id-getter-5.html linux64-shippable-qr opt e10s stylo 476.60 -> 446.65
6% perf_reftest_singletons id-getter-7.html linux64-shippable opt e10s stylo 452.33 -> 424.54
6% perf_reftest_singletons scrollbar-styles-1.html linux64-shippable-qr opt e10s stylo 515.41 -> 484.65
6% perf_reftest_singletons id-getter-7.html linux64-shippable-qr opt e10s stylo 474.48 -> 446.51

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=23969

David, it looks like this basically copied the singletons manifest, but it's already out of sync since more singletons tests got added. The whole point of reftest-singletons is that people should add tests easily, and the expectation is that we should keep doing that.

Is there some technical way to keep the two lists in sync? If not, should we have warning comments around the two lists about keeping them in sync?

Flags: needinfo?(dmajor)

I wasn't aware that these tests were actively changing. I had been getting alerts on the same set of subtests for ages so I assumed they were developed long ago and forgotten. My fault.

I think ideally both raptor and PGO training would load up the same single harness that knows how to iterate over the right set of things. IIRC the current harness assumes the existence of a bunch of raptor/talos machinery so we'd have to factor that out in order to be reused by the PGO run.

Warning comments seem less preferable but might be OK in a pinch.

Flags: needinfo?(dmajor)
Regressions: 1603603
You need to log in before you can comment on or make changes to this bug.