1506763 - Large C++ static initializers

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Vanilla Firefox build (clang):
_GLOBAL__sub_I_Unified_cpp_js_src_gc1.cpp is 6031 bytes of code.
_GLOBAL__sub_I_Unified_cpp_js_src_jit1.cpp is 9898 bytes of code.
Looks like they're just initializing constant data. There are probably others around here. dmajor says:

Looks like they are the `phases` array and all the `FunctionInfo`s,
respectively.

https://searchfox.org/mozilla-central/source/__GENERATED__/js/src/gc/StatsPhasesGenerated.cpp#72
https://searchfox.org/mozilla-central/search?q=FunctionInfo&case=false&regexp=false&path=BaselineCompiler.cpp

I guess converting these to const data (if possible) would reduce binary size and reduce runtime memory usage a little bit.

Comment 1

[(Away)]

While we're at it, those FunctionInfos could really use some decltype love.

Comment 2

[Nicolas B. Pierron [:nbp]]

(In reply to David Major [:dmajor] from comment #1)
> While we're at it, those FunctionInfos could really use some decltype love.

I am not sure to understand the decltype aspect. When this code got added we already had the ability to infer the type from the template parameter.

The reason why we have a an explicit function pointer type was made on purpose, in order to have a local repetition of the function signature, and thus better catch signature changes, than callInfo miss-use.

Comment 3

[(Away)]

Ok, so the GC tables are an easy fix. I'll post a patch.

For the VMFunction issue, we can't completely get away from a runtime constructor, because we have to add each instance to the `functions` linked list. Ideally the compiler would be able to statically-populate the constant fields of the structures, leaving only the minimal work for `functions` setup -- which is 
exactly what I coaxed MSVC into doing in bug 905210 comment 8. But apparently clang wants to do everything by hand at runtime:

mov dword ptr [xul!IonCompileScriptForBaselineInfo+0x20 (00000001`85bfc560)],1000002h
mov   word ptr [xul!IonCompileScriptForBaselineInfo+0x24 (00000001`85bfc564)],100h
etc.

I'll try to distill the repro a bit and file a bug upstream.

Comment 4

[(Away)]

Attachment: Mark [Enumerated]Array constructors as constexpr.

Comment 5

[(Away)]

Attachment: Use constexpr for the GC phase tables.

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 9024788 [details] [diff] [review]
Mark [Enumerated]Array constructors as constexpr.

Review of attachment 9024788 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/Array.h
@@ +26,5 @@
>  public:
>    Array() {}
>  
>    template <typename... Args>
> +  MOZ_IMPLICIT constexpr Array(Args&&... aArgs)

ISTR trying this and it didn't work for some reason, but that might have been either pre-c++14 or pre-MSVC 15.7 or pre-clang-cl...if it works now, that's great!

Comment 7

[Jon Coppeard (:jonco)]

Comment on attachment 9024789 [details] [diff] [review]
Use constexpr for the GC phase tables.

Review of attachment 9024789 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/79148f9b3648
Mark [Enumerated]Array constructors as constexpr. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8bb05d82665
Use constexpr for the GC phase tables. r=jonco

Comment 9

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/79148f9b3648
https://hg.mozilla.org/mozilla-central/rev/d8bb05d82665

Comment 10

[Jan de Mooij [:jandem]]

Bug 1530937 gets rid of the VMFunction/FunctionInfo static initializers FWIW.