1898070 - Consider building Legacy Telemetry in unified mode

Status: RESOLVED FIXED

(Toolkit :: Telemetry, task, P3)

Description

[Chris H-C :chutten]

As reported by Florian:

There are 4 files that don't compile in unified mode because they redefine each other's global variables (gInitDone, gCanRecordBase, gCanRecordExtended). I tried building the rest of the folder unified with this change:

diff --git a/toolkit/components/telemetry/moz.build b/toolkit/components/telemetry/moz.build
--- a/toolkit/components/telemetry/moz.build
+++ b/toolkit/components/telemetry/moz.build
@@ -66,15 +66,17 @@ EXPORTS.mozilla.telemetry += [
 ]
 
 SOURCES += [
+    "core/TelemetryEvent.cpp",
+    "core/TelemetryHistogram.cpp",
+    "core/TelemetryScalar.cpp",
+    "core/TelemetryUserInteraction.cpp",
+]
+UNIFIED_SOURCES += [
     "core/ipc/TelemetryIPC.cpp",
     "core/ipc/TelemetryIPCAccumulator.cpp",
     "core/Stopwatch.cpp",
     "core/Telemetry.cpp",
     "core/TelemetryCommon.cpp",
-    "core/TelemetryEvent.cpp",
-    "core/TelemetryHistogram.cpp",
-    "core/TelemetryScalar.cpp",
-    "core/TelemetryUserInteraction.cpp",
     "dap/DAPTelemetry.cpp",
     "other/CombinedStacks.cpp",
     "other/ProcessedStack.cpp",

It seems to be a bit faster, but I tried only once so I don't know if it's a reproducible improvement or if it was random.

---

This bug is about:

1. Reconsidering the use of globals in those four files (should probably more properly placed inside anonymous namespaces to scope to a file level)
2. Seeing if unified builds are faster
3. If so, land it. If not, document it in t/c/t/moz.build.

Comment 1

[Florian Quèze [:florian]]

Attachment: Bug 1898070 - build Legacy Telemetry C++ files in unified mode, r=chutten.

Comment 2

[Florian Quèze [:florian]]

Build profiles with and without the patch.
With the patch building Unified_cpp_telemetry0.o took 15.9s. Without the patch, the sum of the build times of the separate object files is 55.8s (computed using filteredMarkers.map(m => m.end - m.start).reduce((a,b) => a+b) / 1000 in the browser console on this zoomed view).

The overall build time isn't improved, probably because there are other bottlenecks hiding the win.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a83b1adb62b7
build Legacy Telemetry C++ files in unified mode, r=chutten.

Comment 4

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/a83b1adb62b7

Comment 5

[Florin Bilt]

(In reply to Cristian Tuns from comment #4)

https://hg.mozilla.org/mozilla-central/rev/a83b1adb62b7

Perfherder has detected a build_metrics performance change from push a83b1adb62b79fadb94cdf2a65bb3e30e290ea90.

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
4%	compiler_metrics num_static_constructors	windows2012-aarch64	aarch64	151.00 -> 145.42
4%	compiler_metrics num_static_constructors	windows2012-64	fuzzing	154.00 -> 148.50
3%	compiler_metrics num_static_constructors	windows2012-32	fuzzing	160.00 -> 154.42
3%	compiler_metrics num_static_constructors	linux64	base-toolchains-clang	161.92 -> 156.42
3%	compiler_metrics num_static_constructors	windows2012-64		152.00 -> 147.00
...	...	...	...	...
3%	compiler_metrics num_static_constructors	linux64	base-toolchains	193.92 -> 188.50

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests.

If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a sheriff to do that for you.

You can run these tests on try with ./mach try perf --alert 43265

For more information on performance sheriffing please see our FAQ.