Open Bug 569629 Opened 10 years ago Updated 5 years ago

Get rid of static initializers

Categories

(Core :: General, defect)

x86
Linux
defect
Not set

Tracking

()

People

(Reporter: taras.mozilla, Assigned: glandium)

References

Details

(Whiteboard: [ts][approved-patches-landed][not-ready])

Attachments

(15 files, 8 obsolete files)

37.78 KB, patch
Details | Diff | Splinter Review
31.85 KB, patch
Details | Diff | Splinter Review
100.18 KB, patch
Details | Diff | Splinter Review
10.85 KB, patch
Details | Diff | Splinter Review
3.73 KB, patch
benjamin
: review+
benjamin
: approval2.0+
Details | Diff | Splinter Review
3.01 KB, patch
sicking
: review+
benjamin
: approval2.0+
Details | Diff | Splinter Review
16.31 KB, patch
Details | Diff | Splinter Review
16.97 KB, patch
Details | Diff | Splinter Review
967 bytes, patch
benjamin
: review+
benjamin
: approval2.0+
Details | Diff | Splinter Review
1.80 KB, patch
roc
: review+
benjamin
: approval2.0+
Details | Diff | Splinter Review
1.69 KB, patch
bent.mozilla
: review+
benjamin
: approval2.0+
Details | Diff | Splinter Review
4.55 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
24.69 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
4.71 KB, patch
benjamin
: review-
Details | Diff | Splinter Review
7.46 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
Looks like most of them could be done lazily. They cause extra paging on startup and the impose unnecessary overhead on people who link to libxul. These will be hurtful once bug 561842 lands and people elect to link to libxul to get spidermonkey(ie users like dehydra).
As far as the browser is concerned, on linux static initializers aren't a massive problem because of almost-decent-sized readahead which ensures that code nearby gets read-in for later. On platforms with pathetic readahead such as Windows this might be significant. This should help mobile too.

I blogged about this problem at http://blog.mozilla.com/tglek/2010/05/27/startup-backward-constructors/ and Mike Hommey came up with proof of concept patches.
Whiteboard: [ts]
Attached patch PoC for cycle collection (obsolete) — Splinter Review
Proof of concept for cycle collection. A real patch should imho totally remove NS_IMPL_CYCLE_COLLECTION_CLASS and rename NS_CYCLE_COLLECTION_NAME to avoid risks in third party code.

This patch removes 147 constructors.
Attached patch PoC for Log Modules (obsolete) — Splinter Review
Proof of concept for log modules. This removes 11 constructors.

The patch as it is adds a non negligible overhead to PR_LOG. IMHO, there should be an addition to the nspr API to allow to use static modules declared like this: static PRLogModuleInfo module = { "Name", } (or with a macro, which would be nicer).
With missing parts
Attachment #448969 - Attachment is obsolete: true
In this patch, all static variables were used only on one spot. Moving the static declaration near its use, inside the function, makes the initialization laxy.
Using a function declaring the static variable in its body makes the initialization lazy. The code looks uglier, though.

I haven't entirely made up my mind whether to use static TYPE &foo() or static TYPE *foo(), so I only used the latter when it was necessary.

These could be candidates to a definition of a "singleton" template function somewhere (nscore.h?)
There are several cases where defining a static variable with the value of another creates a static initializer.
There is also the case of const char * const vs. const char []: the mozHunspellDirProvider::kContractID variable is used in the component definition, and if it is defined const char * const, then its pointer has to be copied with a static initializer in the component definition. With const char [], the pointer to the static string can be used directly.
With the five patches attached here, we get down to 32 static initializers. There are a few ones due to the use of iostreams in chromium code, which means except if iostream were to be entirely replaced with something else, it's impossible to remove these static initializers (because of how the class is defined, with a static __ioinit variable).
A bunch of others are due to the preinitialization to 0 of jArrays in the html5 code.
Another is due to the Double::NaN, Double::POSITIVE_INFINITY and Double::NEGATIVE_INFINITY definitions in txDouble.cpp.
Some others due to scoped_ptr_malloc, TLSSlot or base::LazyInstance. In the case of the latter two, the static initializers appear to actually be empty, but when building without optimization, they aren't. Somehow, optimization empties the initializers, but the compiler is unable to remove them accordingly.
Some, like the one due to atomicops_internals_x86_gcc.cc are there by design. Lazy initialization might have a bad impact for such cases.
Some others are less trivial to modify than the ones I already attached, so I didn't bother yet.
> With the five patches attached here, we get down to 32 static initializers.

down from 237, fwiw.
(In reply to comment #7)
> With the five patches attached here, we get down to 32 static initializers.
> There are a few ones due to the use of iostreams in chromium code, which means
> except if iostream were to be entirely replaced with something else, it's
> impossible to remove these static initializers (because of how the class is
> defined, with a static __ioinit variable).

I haven't looked at the chromium code in question, but if it's not using cin, cout, cerr, or clog, it should be possible to get rid of those static initializers by including <istream> and/or <ostream> instead of <iostream>.

(If it does use one of the standard stream objects, yeah, nothing can be done.)
Just out of curiousity, what are the differences in semantics for the
loader here between this:

static PRLogModuleInfo *sCookieLog = PR_NewLogModule("cookie");

And this:

static PRLogModuleInfo *sCookieLog() {
    static PRLogModuleInfo *log = PR_NewLogModule("cookie");
    return log;
}

The old way *must* happen before main() but the second way not?  Does
the second way always compile to code such that PR_NewLogModule is
called when sCookieLog() is first invoked? Is this consistent
across platform/runtime environments?
I hope none of these objects have nontrivial destructors, as we have had problems with that.

(In reply to comment #6)
> There is also the case of const char * const vs. const char []: the
> mozHunspellDirProvider::kContractID variable is used in the component
> definition, and if it is defined const char * const, then its pointer has to be
> copied with a static initializer in the component definition.
Compiler bug? Without optimisations, MSVC compiles const char * const as a load-time reference. With optimisations, MSVC simply uses the string directly.

(In reply to comment #7)
> A bunch of others are due to the preinitialization to 0 of jArrays in the html5 code.
Yeah, that seriously needs a rewrite to make them POD types.
(In reply to comment #10)
> Just out of curiousity...

I cannot find an answer, maybe we get it here: http://stackoverflow.com/questions/3063027/when-exactly-is-constructor-of-static-local-object-called
Cc'ing Henri for comment 11 last para.

/be
(In reply to comment #12)
> (In reply to comment #10)
> > Just out of curiousity...

Got answer: "The object constructor will be run on the first call to a function. This is discussed in Scott Meyer's Effective C++, Item 4. This is enforced by the standard."
(In reply to comment #14)
> (In reply to comment #12)
> > (In reply to comment #10)
> > > Just out of curiousity...
> 
> Got answer: "The object constructor will be run on the first call to a
> function. This is discussed in Scott Meyer's Effective C++, Item 4. This is
> enforced by the standard."

The precise words are (C++98 §6.7p4):

  ... A local object of POD type with static storage duration,
  initialized with *constant-expression*s, is initialized before its
  block is first entered.  An implementation is permitted to perform
  early initialization of other local objects with static storage
  duration under the same conditions that an implementation is
  permitted to statically initialize an object with static storage
  duration in namespace scope (3.6.2). Otherwise such an object is
  initialized the first time control passes through its declaration;
  such an object is considered initialized upon the completion of its
  initialization.

The important piece here is the last sentence: construction is guaranteed not to happen until control reaches the declaration-statement.  (The "permitted to perform early initialization under the same conditions ..." language + cross-reference to section 3.6.2 licenses an implementation to convert a static constructor operation to preinitialized data if the program can't tell the difference, which is not the case for calls to PR_NewLogModule since that has side effects.)

C++98 doesn't contemplate threads, but the C++0x draft I have (from April) adds this language:

  If control enters the declaration concurrently while the variable is
  being initialized, the concurrent execution shall wait for completion
  of the initialization. [footnote: The implementation must not introduce
  any deadlock around execution of the initializer.]

In practice, G++ has emitted thread-safe initialization code for a very long time, and I'd be surprised if MSVC didn't do the same.
Oh, important wrinkle I forgot to mention: If somehow control *recurses* through one of these runtime constructions, the behavior is undefined.  (In practice, this would hang the offending thread forever, as the thread-safety code does not use recursive mutexes.)
Depends on: 502176
Depends on: 573786
Depends on: 579131
(I have other patches in line, but they all rely on a template I want to test more thoroughly and improve for more use cases)

This patch removes the static initializer induced by the kStaticModules initialization in toolkit/library/nsStaticXULComponents.cpp. Although this results in a few more relocations, relocations are still better than a static initializer. The main problem, though, is that it changes how xpcom components are set, again.
Assignee: nobody → mh+mozilla
Attachment #475099 - Flags: review?(benjamin)
(In reply to comment #17)
> This patch removes the static initializer induced by the kStaticModules
> initialization in toolkit/library/nsStaticXULComponents.cpp. Although this
> results in a few more relocations, relocations are still better than a static
> initializer. The main problem, though, is that it changes how xpcom components
> are set, again.

I think you're not describing this change correctly.  mozilla::Module appears to be POD, so it shouldn't generate the kind of static initializer that's bad (executes code before main).  I only skimmed the patch, but what it looks like you're doing is deleting an unnecessary layer of indirection, i.e.

  static mozilla::Module kFoo = { ... }
  mozilla::Module* ExposedFoo = &kFoo;

becomes

  mozilla::Module ExposedFoo = { ... }

which is functionally identical but takes up less space in the data segment (and if anything should induce *fewer* relocations).

So by me this is a righteous change, just perhaps slightly off-topic.

If "mozilla::Module* ExposedFoo = &kFoo" generates a code-ful static constructor, that's a missed optimization in whichever compiler you're using; there's no earthly reason that has to be anything more than a runtime relocation.  (I don't have a current version of GCC on this computer to poke at.  grumble Apple grumble GPL3 grumble **** contest grumble.)
Static initializers are generated by gcc in many cases where the same code in C would only generate relocations. In the present case, as I wrote, it does it to initialize kStaticModules in toolkit/library/nsStaticXULComponents.cpp.
i.e.
static const mozilla::Module *const kStaticModules[] = {
  NSMODULE_NAME(module1),
  NSMODULE_NAME(module2),
  ...
  NULL
};
That seems like a clear bug in GCC('s C++ front end), did you report it?  (If you don't have time, I can do it.)
Actually, I thought this was similar to other cases I saw, but this isn't, and this construct isn't even possible in plain C, gcc barks that the initializer element is not constant. The problem here is transitivity.

static mozilla::Module kFoo = { ... }
mozilla::Module* ExposedFoo = &kFoo;
mozilla::Module* kStaticModules[] = {
  ExposedFoo,
  NULL
};

The only way this can be initialized is by reading &kFoo from ExposedFoo, and writing it in kStaticModules[0]. While the compiler could take shortcuts when everything is defined in the same module, it certainly can't when it's not the case, except maybe with LTO.
In this particular case we can change kPStaticModules to use an extra level of indirection (store ** instead of *) to avoid the problem, I bet.
(In reply to comment #22)
> In this particular case we can change kPStaticModules to use an extra level of
> indirection (store ** instead of *) to avoid the problem, I bet.

If you don't want to change the xpcom modules ABI yet again, yes, that would work.
Not for this release I don't ;-)
Less disruptive version, as suggested in comment 22. For Justin, who was concerned with the impact on c-c, this version shouldn't need any change to c-c.
Attachment #475099 - Attachment is obsolete: true
Attachment #475786 - Flags: review?(benjamin)
Attachment #475099 - Flags: review?(benjamin)
Same as the previous one, but saves a useless relocation.
Attachment #475786 - Attachment is obsolete: true
Attachment #475817 - Flags: review?(benjamin)
Attachment #475786 - Flags: review?(benjamin)
(In reply to comment #27)
> I pushed a test to try with roughyl the same as attachment 448968 [details] [diff] [review], but I need
> to make sense of the Talos differences compared to the same tree without the
> patches (any help appreciated):
getting a bug on file within 24 hours of your push to get more talos runs for the tests you care about (I've found three to be a good number to minimize noise) has been handy for me.  Using more than one oldRev is handy too.
Same patch, with mq headers.
Attachment #475817 - Attachment is obsolete: true
Attachment #481432 - Flags: review?(benjamin)
Attachment #475817 - Flags: review?(benjamin)
The current code initializes unions consts, then copies the union value in a double. This isn't optimized by gcc, but using a cast operator is. I don't know if MSVC does the right thing.
Attachment #481433 - Flags: review?(benjamin)
My first try here was to simply get rid of the static initializer by implementing a raw ContextFormat struct from which ContextFormat would derive, and thus avoid the call to the constructor. But in the end, it also turns out the code to fill the struct dynamically is actually bigger than the struct itself, so storing the different values as consts shouldn't be wasteful.

It also appears BasicRGB16_565 was not defined.
Attachment #481434 - Flags: review?(benjamin)
GLXLibrary and OSMesaLibrary are only used each in one file and with one instance. Making them a class with a constructor makes their initialization use a static initializer. Using a plain struct allows to have them stored in .bss (since all values are 0). Since GLXLibrary is only used in one file, I also moved its definition to the cpp file instead of having it exported in a header file (which is also why Vladimir's name pops up in the patch).

Another approach would have been to use a pointer, but that would add an indirection through the pointer, and a constructor which only nulls the struct out.
Attachment #481435 - Flags: review?(benjamin)
Using std::numeric_limits<>::max() uses a static initializer while using the equivalent constant doesn't. Note that UINT_MAX and the like could be used instead of a casted -1.
Attachment #481436 - Flags: review?(benjamin)
This is similar to OSMesaLibrary and GLXLibrary.

Note that gCaptureInfo isn't apparently used outside nsPresShell.cpp, so maybe the struct definition could be moved there, too.
Attachment #481439 - Flags: review?(benjamin)
Pretty similar to nsIPresShell::gCaptureInfo, OSMesaLibrary and GLXLibrary, except this time, this doesn't end up in .bss. I haven't taken a look at the code using the returned JSPrincipals, so I don't know if that could be made a const or not (probably not).
Attachment #481440 - Flags: review?(benjamin)
This one doesn't remove static initializers but is going to be used by subsequent patches to avoid static initializers. This implements a template deferring the class constructor until the instance is used for the first time.

It is not thread safe, but I verified that I didn't need it to be.
We could get rid of the storage/memory overhead for classes with a single vtable and no constructor, using the vtable itself as a check, but there aren't too many cases that would fit. Anything else would be race risky, and introducing thread safety would add (imho unnecessary) complexity.
Attachment #481446 - Flags: review?(benjamin)
Attachment #448968 - Attachment is obsolete: true
Attachment #481448 - Flags: review?(benjamin)
Defining global instances of classes with vtables unfortunately creates a static initializer. Using the Lazy initializer helps getting rid of them.
Attachment #481450 - Flags: review?(benjamin)
(In reply to comment #37)
> Created attachment 481448 [details] [diff] [review]
> Use Lazy initialization for cycle collector

I forgot to comment on this one. This is the patch that removes the most static initializers, somewhere between 170 and 180 (out of, currently, 297).

I tried several approaches for the Lazy initialization template, one of which was using a static variable in the get() function, which had thread safety, but on the downside didn't allow several instances of the same class, but it turned out all cases I was covering are thread safe, thus the current thread non-safe version.

I was afraid fiddling with cycle collection could affect performance, but it turned out not to be so bad:
http://perf.snarkfest.net/compare-talos/index.html?oldRevs=c257bfb8cad0,b75b24a337b3,faf497893b28,a7300f9d11ae,f584b2db2a7e&newRev=521af4659410&tests=a11y,tdhtml,tdhtml_nochrome,tp4,tp4_memset,tp4_pbytes,tp4_rss,tp4_shutdown,tp4_xres,dromaeo_basics,dromaeo_css,dromaeo_dom,dromaeo_jslib,dromaeo_sunspider,dromaeo_v8,tsspider,tsspider_nochrome,tgfx,tgfx_nochrome,tscroll,tsvg,tsvg_opacity,ts,ts_cold,ts_cold_generated_max,ts_cold_generated_max_shutdown,ts_cold_generated_med,ts_cold_generated_med_shutdown,ts_cold_generated_min,ts_cold_generated_min_shutdown,ts_cold_shutdown,ts_places_generated_max,ts_places_generated_max_shutdown,ts_places_generated_med,ts_places_generated_med_shutdown,ts_places_generated_min,ts_places_generated_min_shutdown,ts_shutdown,twinopen&submit=true
(In reply to comment #38)
> Created attachment 481450 [details] [diff] [review]
> Use Lazy initialization in various places
> 
> Defining global instances of classes with vtables unfortunately creates a
> static initializer. Using the Lazy initializer helps getting rid of them.

I should also mention that I made sure all these are thread safe. At most, the initialization writes vtables and a refcounter that is not used in these classes (those that have a refcounter have custom AddRef and Release functions returning a static value), and calls no constructor. This means that in the worst racy case where several threads get in the initialization code, all that can happen in a thread overwriting what the other wrote with the same information.

Well, the above is with GCC, I'm not entirely sure what MSVC generates there, but it shouldn't be very different.
This one is similar to the other Lazy initialization ones, but I separated it out because it hacks the lazy initialization template itself to also get rid of a static bool storing the initialization state.

With the attachments I posted today, we're down from 297 to 93 on linux. 11 more can be removed for log modules (I'll post something on dev-platform today about that), and I also have around 15 more that should be easily removable, but the current patches I have use the old implementation of the Lazy initialization template, and are changing things they shouldn't. I also need to go through the PoC attachments again to check the ones I forgot in the way.

Please note that, for the record, another bunch of static initializers come from libmozjs and are most due to JS_DEFINE_CALLINFO.
Attachment #481452 - Flags: review?(benjamin)
Depends on: 602467
I should also mention that from my measurement on a usb hard drive, applying all these patches reduces cold startup by ~ 200ms off of ~4s (without omnijar reordering).
Whee, that's a lot of patches.

I volunteer to take reviews for all these if bsmedberg's busy. 5% of cold startup is a nice win indeed.
While looking at a better patch for the SMIL/SVG static initializers, I figured the current Lazy implementation was not really helpful there, and as it form was more or less due to its history of being much more complex before, it just made sense to simplify it and also make it easier to use with classes with protected constructors as a side effect.
Attachment #481446 - Attachment is obsolete: true
Attachment #481797 - Flags: review?(benjamin)
Attachment #481446 - Flags: review?(benjamin)
Updated to fit the new Lazy implementation
Attachment #481452 - Attachment is obsolete: true
Attachment #481798 - Flags: review?(benjamin)
Attachment #481452 - Flags: review?(benjamin)
This one saves 6 static initializers (though due to another one in one of the files, it's actually 5). FWIW, SVG has a pretty similar setup which could get a similar patch.

But now that I think more of it, I'm reluctant to use Lazy initialization here: AFAIUI, nsISMILTypes are "simply" function tables. The fact that it ends up requiring a static initializer to copy the vtable pointer over is quite sad. So, instead of using Lazy initialization, replacing these classes with actual function tables would save these static initializers. That would even save an indirect pointer due to the vtable. Any comments ?

(Arguably, we could also get gcc to avoid using static initializers at all for such cases...)
Attachment #481799 - Flags: review?(benjamin)
Attachment #481433 - Flags: review?(benjamin) → review?(jonas)
Attachment #481434 - Flags: review?(benjamin) → review?(vladimir)
Attachment #481435 - Flags: review?(benjamin) → review?(vladimir)
Attachment #481439 - Flags: review?(benjamin) → review?(roc)
Attachment #481440 - Flags: review?(benjamin) → review?(jonas)
Comment on attachment 481799 [details] [diff] [review]
Use Lazy initialization for SMIL types

I'd like feedback on comment 46.
Attachment #481799 - Flags: review?(benjamin) → feedback?(roc)
Yes, I think it would make sense to use function tables instead.
(Although it's kinda sad we have to jump through these hoops.)
Attachment #481440 - Flags: review?(jonas) → review?(bent.mozilla)
Attachment #481440 - Flags: review?(bent.mozilla) → review+
(In reply to comment #49)
> (Although it's kinda sad we have to jump through these hoops.)

Well, it should be possible to use non static variables for that, and compare their vtables/typeid, but it either requires rtti or ugly hacks (though xpcom already has such ugly hacks).
Attachment #481432 - Flags: review?(benjamin)
Attachment #481432 - Flags: review+
Attachment #481432 - Flags: approval2.0+
Attachment #481436 - Flags: review?(benjamin)
Attachment #481436 - Flags: review+
Attachment #481436 - Flags: approval2.0+
Attachment #481433 - Flags: approval2.0?
Attachment #481439 - Flags: approval2.0?
Attachment #481440 - Flags: approval2.0?
Attachment #481433 - Flags: approval2.0? → approval2.0+
Attachment #481439 - Flags: approval2.0? → approval2.0+
Attachment #481440 - Flags: approval2.0? → approval2.0+
Comment on attachment 481432 [details] [diff] [review]
Remove static xpcom modules list static initializer [checked-in]

http://hg.mozilla.org/mozilla-central/rev/73389f240593
Attachment #481432 - Attachment description: Remove static xpcom modules list static initializer → Remove static xpcom modules list static initializer [checked-in]
Comment on attachment 481433 [details] [diff] [review]
Modify Double::NaN,POSITIVE_INFINITY,NEGATIVE_INFINITY to avoid static initializers [checked-in]

http://hg.mozilla.org/mozilla-central/rev/831b92ba7fc5
Attachment #481433 - Attachment description: Modify Double::NaN,POSITIVE_INFINITY,NEGATIVE_INFINITY to avoid static initializers → Modify Double::NaN,POSITIVE_INFINITY,NEGATIVE_INFINITY to avoid static initializers [checked-in]
Comment on attachment 481436 [details] [diff] [review]
Don't use std::numeric_limits to initialize static variables [checked-in]

http://hg.mozilla.org/mozilla-central/rev/abcec14992b1
Attachment #481436 - Attachment description: Don't use std::numeric_limits to initialize static variables → Don't use std::numeric_limits to initialize static variables [checked-in]
Comment on attachment 481439 [details] [diff] [review]
Initialize nsIPresShell::gCaptureInfo without a constructor [checked-in]

http://hg.mozilla.org/mozilla-central/rev/df36c2dcf88f
Attachment #481439 - Attachment description: Initialize nsIPresShell::gCaptureInfo without a constructor → Initialize nsIPresShell::gCaptureInfo without a constructor [checked-in]
Comment on attachment 481440 [details] [diff] [review]
Don't use a constructor for nsDOMWorkerPrincipal [checked-in]

http://hg.mozilla.org/mozilla-central/rev/0a9ee83b1093
Attachment #481440 - Attachment description: Don't use a constructor for nsDOMWorkerPrincipal → Don't use a constructor for nsDOMWorkerPrincipal [checked-in]
Depends on: 616262
Attachment #481799 - Flags: feedback?(roc)
Whiteboard: [ts] → [ts][approved-patches-landed]
Whiteboard: [ts][approved-patches-landed] → [ts][approved-patches-landed][not-ready]
Comment on attachment 481797 [details] [diff] [review]
Define a template allowing to defer a class constructor until the instance gets used for the first time

Apart from the question about alignment I asked on IRC, I don't understand why it's necessary to have two classes (Lazy and LazyImpl). Couldn't you just make it:

template<class Type>
class Lazy {
  bool mInitialized;
  char buf[sizeof<Type>] mBuffer; // alignment here somehow?

  Type& get() { return reinterpret_cast<Type*>(mBuffer); }
  etc...
};

Also, I don't think we'd ever want a "static const Lazy<Foo>": wouldn't the data end up in a readonly section? So I think all the mutable/const bits can be removed, unless I misunderstand what they're for.
Can you use mozilla::Maybe (in mfbt/Util.h)?
(In reply to comment #57)
> Can you use mozilla::Maybe (in mfbt/Util.h)?

Oh it looks like it does more or less the same thing.
Attachment #481797 - Flags: review?(benjamin) → review-
Attachment #481448 - Flags: review?(benjamin) → review+
Comment on attachment 481450 [details] [diff] [review]
Use Lazy initialization in various places

all these reviews can be tranferred to equivalent patches that use MFBT
Attachment #481450 - Flags: review?(benjamin) → review+
Comment on attachment 481798 [details] [diff] [review]
Use Lazy initialization for MemoryElement::gPool

I suspect we'd be better off just removing this pool entirely, but r=me if this is easier.
Attachment #481798 - Flags: review?(benjamin) → review+
Comment on attachment 481799 [details] [diff] [review]
Use Lazy initialization for SMIL types

Obsoleted (and fixed by bug 903543)
Attachment #481799 - Attachment is obsolete: true
Nathan, since you've done some static initializer work recently, would you mind looking at the patches that haven't landed from this bug, and see if they could be replaced with some constexpr? (I think they can)
Flags: needinfo?(nfroyd)
I didn't actually check whether these patches applied (PRBool, at least, has bitrotted some of them), just whether they'd be improved by constexpr.

The bits in attachment 448974 [details] [diff] [review] look like they're still valid; I don't think any of those can be replaced with constexpr.

The bits in attachment 448978 [details] [diff] [review] are mostly still valid; I think we've mostly taken care of the non-TearoffTable stuff in SMIL &co., but everything else seems OK.

Attachment 448979 [details] [diff] looks like a hodgepodge of non-constexpr related stuff.

Attachment 481435 [details] [diff] looks like a case for constexpr.

Attachment 481434 [details] [diff] looks like it applies as-is.
Flags: needinfo?(nfroyd)
Comment on attachment 481434 [details] [diff] [review]
Define GL ContextFormats as const instead of dynamically generated structs

Clearing ancient review request (sorry!).. if this is still relevant, r? again pls.  For this code, jgilbert or bjacob are better.
Attachment #481434 - Flags: review?(vladimir)
You need to log in before you can comment on or make changes to this bug.