Closed Bug 809306 Opened 7 years ago Closed 6 years ago

remove duplicate static initializers caused by IonMonkey's assembler headers

Categories

(Core :: JavaScript Engine, defect)

All
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: froydnj, Unassigned)

Details

Attachments

(2 files, 2 obsolete files)

Attached patch patch (obsolete) — Splinter Review
ion/$ARCH/Assembler-$ARCH.h has lots of definitions like:

static const Register rax = { JSC::X86Registers::eax };

This causes several problems:

1) static variable definitions don't really belong in header files; they're fine (I guess) if the data is POD (and therefore the compiler can inline it all), but...
2) the definition in this case is not POD and GCC doesn't understand how to generate static data for such definitions (clang does; I don't know about MSVC), so this generates a static initializer; and
3) since this is all in a *header* file, you get a separate static initializer for every variable in every file in which the header is #included.  Not to mention the wasted space of duplicate definitions.

The upshot is this scheme wastes about 20K of code (on x86-64) and some ~4-8K of data due to all the duplicate definitions.

Stop the madness!  Attached patch seems to do the trick for me on x86-64; untested on other architectures.  Getting rid of the static initializers totally is a big job; this is a step in the right direction by making sure there's only one set of them.
Attachment #679019 - Flags: review?(luke)
(In reply to Nathan Froyd (:froydnj) from comment #0)
> (clang does; I don't know about MSVC)

FYI, MSVC usually does like clang, but under some circumstances (haven't researched which), it doesn't.
Comment on attachment 679019 [details] [diff] [review]
patch

Forwarding review request.
Attachment #679019 - Flags: review?(luke) → review?(sstangl)
Attached patch patch v2 (obsolete) — Splinter Review
Here's a version that compiles on ARM, according to try.
Attachment #679019 - Attachment is obsolete: true
Attachment #679019 - Flags: review?(sstangl)
Attachment #679231 - Flags: review?(sstangl)
Attached patch patch v3Splinter Review
I think this one is a little better; CodeGenerator.cpp's static initializer
referred to OsrFrameReg, which I *think* is now not guaranteed to be
initialized at that point.  Since EntryTempMask is only used in one function,
just move it there, which happily eliminates another static initializer.
Attachment #679231 - Attachment is obsolete: true
Attachment #679231 - Flags: review?(sstangl)
Attachment #679318 - Flags: review?(sstangl)
This patch adds quite a bit of source-level duplication. Is the benefit purely shaving 24KB off the binary size?
(In reply to Nathan Froyd (:froydnj) from comment #4)
> Created attachment 679318 [details] [diff] [review]
> patch v3
> 
> I think this one is a little better; CodeGenerator.cpp's static initializer
> referred to OsrFrameReg, which I *think* is now not guaranteed to be
> initialized at that point.  Since EntryTempMask is only used in one function,
> just move it there, which happily eliminates another static initializer.

An option to avoid code duplication is to typedef Register and FloatRegister to integers in opt builds, and have a macro constructor: the only point of the Register and FloatRegister classes is to provide type safety by wrapping integers, but we only need type safety for debug builds, and we only would like savings on the binary size in opt builds.

This would also allow the compiler to actually make optimizations with constant integers, where with non-POD structs it cannot.
(In reply to David Anderson [:dvander] from comment #5)
> This patch adds quite a bit of source-level duplication. Is the benefit
> purely shaving 24KB off the binary size?

Is the duplication you're referring to the necessity of declaring and defining the registers in separate locations, or something else?  That doesn't seem like any more duplication than the usual declare-and-define dance for functions.

(In reply to Sean Stangl from comment #6)
> An option to avoid code duplication is to typedef Register and FloatRegister
> to integers in opt builds, and have a macro constructor: the only point of
> the Register and FloatRegister classes is to provide type safety by wrapping
> integers, but we only need type safety for debug builds, and we only would
> like savings on the binary size in opt builds.
> 
> This would also allow the compiler to actually make optimizations with
> constant integers, where with non-POD structs it cannot.

That's not a bad idea, and likely preserves the status quo for clang and maybe MSVC.  I'll poke at implementing that.
Comment on attachment 679318 [details] [diff] [review]
patch v3

A brief look at the Ion code suggests trying the alternate proposal of:

#ifdef DEBUG
class Register { ... };
#else
typedef int Register;
#endif

would be a rather large undertaking.
Attachment #679318 - Flags: review?(sstangl)
Comment on attachment 759900 [details] [diff] [review]
bug 809306 - fix a bunch of static initializers caused by ion monkey's assembler headers

Does it work to MOZ_CONSTEXPR the Register/FloatRegister constructors, or do you have to MOZ_CONSTEXPR the variable declarations?  The former would be a lot less noisy if that worked.
Attachment #759900 - Flags: review?(dvander) → review+
https://hg.mozilla.org/mozilla-central/rev/e213c2a01ec2
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
This broke building Fennec:

js/src/ion/arm/Assembler-arm.h:<various lines>:38: error: both 'const' and 'constexpr' cannot be used here
(In reply to Mike Hommey (high latency until June 25) [:glandium] from comment #14)
> This broke building Fennec:
> 
> js/src/ion/arm/Assembler-arm.h:<various lines>:38: error: both 'const' and
> 'constexpr' cannot be used here

(with gcc < 4.7)
You need to log in before you can comment on or make changes to this bug.