1737028 - 3.77% compiler_metrics num_static_constructors (Windows) regression on Wed October 20 2021

Status: RESOLVED WONTFIX

(Core :: Internationalization, defect)

Description

[Andra Esanu (needinfo me)]

Perfherder has detected a build_metrics performance regression from push 0f203e8e17f9f7a17124191e3e33d05e83c90f27. As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

Ratio	Test	Platform	Options	Absolute values (old vs new)
4%	compiler_metrics num_static_constructors	windows2012-64		106.00 -> 110.00

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests. Please follow our guide to handling regression bugs and let us know your plans within 3 business days, or the offending patch(es) will be backed out in accordance with our regression policy.

For more information on performance sheriffing please see our FAQ.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1719546

Comment 2

[Greg Tatum [:gregtatum]]

I believe this is due to header includes. I'll look at splitting out the Bidi::EmbeddingLevel into its own header to minimizes the impact.

Comment 3

[Greg Tatum [:gregtatum]]

Actually this is the raw count of static initializers, not build time. I'm trying to understand what exactly this alert means and if it's a real regression to care about.

Comment 4

[Greg Tatum [:gregtatum]]

I am going to suggest this issue be closed, as the regression is not a performance regression. In Bug 1464522 this alert was added, and reading through it, it was mostly added as a mechanism to get a list of static initializers in C++, and to understand their usages. Bug 1719546 changed some of the implementation of Bidi, which is used in various places in layout and change the generated code. I don't see any reason why this would be an issue, and seems to be a somewhat arbitrary numeric change.

:dminor do you agree with my assessment and decision to close as WONTFIX?

I did a try run listing all of the names, and here they are below:

https://treeherder.mozilla.org/jobs?repo=try&revision=7891ab9ecf0aba7b717205a809d271968639cb2d&selectedTaskRun=GEm_zRYRT_uHTePt9GbbYw.0

_GLOBAL__sub_I_sandboxBroker.cpp()
_GLOBAL__sub_I_Unified_cpp_xpcom_base2.cpp()
_GLOBAL__sub_I_Unified_cpp_xpcom_threads1.cpp()
_GLOBAL__sub_I_PoisonIOInterposerWin.cpp()
_GLOBAL__sub_I_Unified_cpp_modules_libpref0.cpp()
_GLOBAL__sub_I_Unified_cpp_intl_locale0.cpp()
_GLOBAL__sub_I_Unified_cpp_converters0.cpp()
_GLOBAL__sub_I_Unified_cpp_protocol_http2.cpp()
_GLOBAL__sub_I_Unified_cpp_protocol_http3.cpp()
_GLOBAL__sub_I_Unified_cpp_url_classifier0.cpp()
_GLOBAL__sub_I_process_util_win.cc()
_GLOBAL__sub_I_Unified_cpp_ipc_glue1.cpp()
_GLOBAL__sub_I_SpinEvent.cpp()
_GLOBAL__sub_I_Unified_cpp_hal0.cpp()
_GLOBAL__sub_I_dtlsidentity.cpp()
_GLOBAL__sub_I_Unified_cpp_src_lib_json0.cpp()
_GLOBAL__sub_I_NativeFontResourceDWrite.cpp()
_GLOBAL__sub_I_Unified_cpp_gfx_gl0.cpp()
_GLOBAL__sub_I_Unified_cpp_gfx_layers0.cpp()
_GLOBAL__sub_I_Unified_cpp_gfx_layers3.cpp()
_GLOBAL__sub_I_Unified_cpp_gfx_layers4.cpp()
_GLOBAL__sub_I_Unified_cpp_gfx_layers6.cpp()
_GLOBAL__sub_I_Unified_cpp_gfx_layers7.cpp()
_GLOBAL__sub_I_gfxDWriteCommon.cpp()
_GLOBAL__sub_I_gfxFontUtils.cpp()
rlbox::rlbox_sandbox<rlbox::rlbox_wasm2c_sandbox>::`dynamic initializer for 'sandbox_list_lock'()
rlbox::rlbox_sandbox<rlbox::rlbox_wasm2c_sandbox>::`dynamic initializer for 'sandbox_list'()
_GLOBAL__sub_I_Unified_cpp_gfx_thebes0.cpp()
_GLOBAL__sub_I_Unified_cpp_gfx_thebes1.cpp()
_GLOBAL__sub_I_openvr_api_public.cpp()
_GLOBAL__sub_I_Unified_cpp_gfx_config0.cpp()
_GLOBAL__sub_I_Unified_cpp_webrender_bindings0.cpp()
_GLOBAL__sub_I_nsContentUtils.cpp()
_GLOBAL__sub_I_nsDOMWindowUtils.cpp()
_GLOBAL__sub_I_Unified_cpp_dom_base0.cpp()
_GLOBAL__sub_I_Unified_cpp_dom_base2.cpp()
_GLOBAL__sub_I_Unified_cpp_dom_base4.cpp()
_GLOBAL__sub_I_Unified_cpp_dom_base5.cpp()
_GLOBAL__sub_I_Unified_cpp_dom_base9.cpp()
_GLOBAL__sub_I_Unified_cpp_dom_bindings0.cpp()
_GLOBAL__sub_I_Unified_cpp_dom_canvas1.cpp()
_GLOBAL__sub_I_Unified_cpp_dom_canvas4.cpp()
_GLOBAL__sub_I_Unified_cpp_dom_crypto0.cpp()
_GLOBAL__sub_I_EventStateManager.cpp()
_GLOBAL__sub_I_Unified_cpp_dom_events1.cpp()
_GLOBAL__sub_I_Unified_cpp_dom_events3.cpp()
_GLOBAL__sub_I_Unified_cpp_dom_fetch0.cpp()
_GLOBAL__sub_I_Unified_cpp_dom_gamepad0.cpp()
_GLOBAL__sub_I_Unified_cpp_dom_html1.cpp()
_GLOBAL__sub_I_Unified_cpp_dom_html_input0.cpp()
_GLOBAL__sub_I_CubebUtils.cpp()
_GLOBAL__sub_I_Unified_cpp_dom_media3.cpp()
_GLOBAL__sub_I_Unified_cpp_dom_media_doctor0.cpp()
_GLOBAL__sub_I_Unified_cpp_dom_media_gmp0.cpp()
_GLOBAL__sub_I_RemoteDecoderManagerChild.cpp()
rlbox::rlbox_sandbox<rlbox::rlbox_noop_sandbox>::`dynamic initializer for 'sandbox_list_lock'()
rlbox::rlbox_sandbox<rlbox::rlbox_noop_sandbox>::`dynamic initializer for 'sandbox_list'()
_GLOBAL__sub_I_Unified_cpp_systemservices0.cpp()
_GLOBAL__sub_I_Unified_cpp_media_webrtc_jsapi0.cpp()
_GLOBAL__sub_I_Unified_cpp_libwebrtcglue0.cpp()
_GLOBAL__sub_I_RsdparsaSdpAttributeList.cpp()
_GLOBAL__sub_I_Unified_cpp_dom_media_webrtc_sdp0.cpp()
_GLOBAL__sub_I_Unified_cpp_dom_system0.cpp()
_GLOBAL__sub_I_ProcessHangMonitor.cpp()
_GLOBAL__sub_I_Unified_cpp_dom_ipc0.cpp()
_GLOBAL__sub_I_Unified_cpp_dom_workers0.cpp()
_GLOBAL__sub_I_Unified_cpp_dom_promise0.cpp()
_GLOBAL__sub_I_Unified_cpp_dom_webauthn0.cpp()
_GLOBAL__sub_I_Unified_cpp_dom_xhr0.cpp()
_GLOBAL__sub_I_Unified_cpp_dom_serviceworkers1.cpp()
_GLOBAL__sub_I_Unified_cpp_dom_localstorage0.cpp()
_GLOBAL__sub_I_nsBaseWidget.cpp()
_GLOBAL__sub_I_Unified_cpp_widget0.cpp()
_GLOBAL__sub_I_Unified_cpp_widget3.cpp()
_GLOBAL__sub_I_nsFilePicker.cpp()
_GLOBAL__sub_I_Unified_cpp_widget_windows0.cpp()
_GLOBAL__sub_I_Unified_cpp_widget_windows3.cpp()
_GLOBAL__sub_I_Unified_cpp_layout_painting0.cpp()
_GLOBAL__sub_I_SkScalerContext_win_dw.cpp()
_GLOBAL__sub_I_Unified_cpp_audio_processing_gn0.cpp()
_GLOBAL__sub_I_Unified_cpp__approved_generic_gn1.cpp()
_GLOBAL__sub_I_Unified_cpp__approved_generic_gn2.cpp()
_GLOBAL__sub_I_Unified_cpp_docshell_shistory0.cpp()
_GLOBAL__sub_I_Unified_cpp_windows_msaa0.cpp()
_GLOBAL__sub_I_PlatformChild.cpp()
_GLOBAL__sub_I_Unified_cpp_tools_profiler0.cpp()
_GLOBAL__sub_I_Unified_cpp_tools_profiler1.cpp()
_GLOBAL__sub_I_Unified_cpp_hunspell_glue0.cpp()
_GLOBAL__sub_I_Unified_cpp_security_manager_ssl1.cpp()
_GLOBAL__sub_I_Unified_cpp_security_manager_ssl2.cpp()
_GLOBAL__sub_I_Unified_cpp_antitracking0.cpp()
_GLOBAL__sub_I_status.cc()
_GLOBAL__sub_I_TelemetryEvent.cpp()
_GLOBAL__sub_I_TelemetryOrigin.cpp()
_GLOBAL__sub_I_TelemetryScalar.cpp()
_GLOBAL__sub_I_GeckoViewStreamingTelemetry.cpp()
_GLOBAL__sub_I_TelemetryIOInterposeObserver.cpp()
_GLOBAL__sub_I_Unified_cpp_url_classifier0.cpp()
_GLOBAL__sub_I_Unified_cpp_components_nimbus0.cpp()
_GLOBAL__sub_I_Unified_cpp_crashreporter0.cpp()
_GLOBAL__sub_I_nsAppRunner.cpp()
_GLOBAL__sub_I_Unified_cpp_toolkit_xre1.cpp()
_GLOBAL__sub_I_Unified_cpp_pref_autoconfig_src0.cpp()
_GLOBAL__sub_I_Unified_cpp_js_src10.cpp()
_GLOBAL__sub_I_Unified_cpp_js_src12.cpp()
_GLOBAL__sub_I_Unified_cpp_js_src18.cpp()
_GLOBAL__sub_I_Unified_cpp_js_src24.cpp()
js::`dynamic initializer for 'wellKnownAtomInfos'()
_GLOBAL__sub_I_Unified_cpp_js_src28.cpp()
_GLOBAL__sub_I_Unified_cpp_js_src_jit6.cpp()
_GLOBAL__sub_I_Unified_cpp_js_src_wasm1.cpp()
_GLOBAL__sub_I_Unified_cpp_js_src_jit5.cpp()
_GLOBAL__sub_I_Unified_cpp_js_src_wasm4.cpp()
_GLOBAL__sub_I_Unified_cpp_js_src_wasm2.cpp()
_GLOBAL__sub_I_Unified_cpp_js_src_jit8.cpp()
_GLOBAL__sub_I_Unified_cpp_js_src_jit2.cpp()
std::`dynamic initializer for '_Fac_tidy_reg''()

Comment 5

[Dan Minor [:dminor]]

The build system docs say we're trying to keep the number of static constructors as low as possible: https://firefox-source-docs.mozilla.org/performance/build_metrics/build_metrics.html, but I agree the numbers here are kind of arbitrary.

Looking at the patch, I wonder if the culprit is: https://searchfox.org/mozilla-central/search?q=BIDI_LEVEL_UNDEFINED&path=&case=false&regexp=false. Could you maybe replace the #define with a constexpr or add a BidiLevelUndefined() static method to mozilla::intl::Bidi::EmbeddingLevel?

Comment 6

[Dan Minor [:dminor]]

Greg pointed out the #define probably isn't expanded in the static scope, so it's not clear to me why this patch would have changed things.

Comment 7

[Greg Tatum [:gregtatum]]

I did a comparison on a non-unified build and there was only 1 additional one, and it was in _GLOBAL__sub_I_nsFilePicker.cpp().

Comment 8

[Greg Tatum [:gregtatum]]

Considering this internationalization API is touching some core layout primitives, and the compiler can generate static initializers even if they are not explicitly specified, I'm not sure there is anything actionable I can do here. I feel like I've done my due diligence investigating the source, and nothing explicitly stands out. My patch is not introducing new static initialization sites, and so the compiler may be optimizing the code different. In chat dminor agreed with my decision to WONTFIX this. I'd be happy to re-investigate if anyone disagrees with my analysis.

Filter for "!!!greg" to see the names of the initializers:

https://treeherder.mozilla.org/logviewer?job_id=355648422&repo=try
https://treeherder.mozilla.org/logviewer?job_id=355649068&repo=try