(dormant account) Reporter
	Description • 15 years ago

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.

	Mike Hommey [:glandium] Assignee
	Comment 1 • 15 years ago

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.

	Mike Hommey [:glandium] Assignee
	Comment 2 • 15 years ago

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).

	Mike Hommey [:glandium] Assignee
	Comment 3 • 15 years ago

With missing parts

	Mike Hommey [:glandium] Assignee
	Comment 4 • 15 years ago

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.

	Mike Hommey [:glandium] Assignee
	Comment 5 • 15 years ago

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?)

	Mike Hommey [:glandium] Assignee
	Comment 6 • 15 years ago

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.

	Mike Hommey [:glandium] Assignee
	Comment 7 • 15 years ago

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.

	Mike Hommey [:glandium] Assignee
	Comment 8 • 15 years ago

> With the five patches attached here, we get down to 32 static initializers.

down from 237, fwiw.

	Zack Weinberg (:zwol)
	Comment 9 • 15 years ago

(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.)

	John Daggett (:jtd)
	Comment 10 • 15 years ago

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?

	neil@parkwaycc.co.uk
	Comment 11 • 15 years ago

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.

	Honza Bambas (:mayhemer)
	Comment 12 • 15 years ago

(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

	Brendan Eich [:brendan]
	Comment 13 • 15 years ago

Cc'ing Henri for comment 11 last para.

/be

	Honza Bambas (:mayhemer)
	Comment 14 • 15 years ago

(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."

	Zack Weinberg (:zwol)
	Comment 15 • 15 years ago

(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.

	Zack Weinberg (:zwol)
	Comment 16 • 15 years ago

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.)

	Mike Hommey [:glandium] Assignee
	Comment 17 • 14 years ago

(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.

	Zack Weinberg (:zwol)
	Comment 18 • 14 years ago

(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.)

	Mike Hommey [:glandium] Assignee
	Comment 19 • 14 years ago

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
};

	Zack Weinberg (:zwol)
	Comment 20 • 14 years ago

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.)

	Mike Hommey [:glandium] Assignee
	Comment 21 • 14 years ago

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.

	Benjamin Smedberg
	Comment 22 • 14 years ago

In this particular case we can change kPStaticModules to use an extra level of indirection (store ** instead of *) to avoid the problem, I bet.

	Mike Hommey [:glandium] Assignee
	Comment 23 • 14 years ago

(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.

	Benjamin Smedberg
	Comment 24 • 14 years ago

Not for this release I don't ;-)

	Mike Hommey [:glandium] Assignee
	Comment 25 • 14 years ago

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.

	Mike Hommey [:glandium] Assignee
	Comment 26 • 14 years ago

Same as the previous one, but saves a useless relocation.

	Mike Hommey [:glandium] Assignee
	Comment 27 • 14 years ago

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):
http://perf.snarkfest.net/compare-talos/index.html?oldRevs=c257bfb8cad0&newRev=7f5b705cbc39&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

	Shawn Wilsher :sdwilsh
	Comment 28 • 14 years ago

(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.

	Mike Hommey [:glandium] Assignee
	Comment 29 • 14 years ago

Same patch, with mq headers.

	Mike Hommey [:glandium] Assignee
	Comment 30 • 14 years ago

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.

	Mike Hommey [:glandium] Assignee
	Comment 31 • 14 years ago

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.

	Mike Hommey [:glandium] Assignee
	Comment 32 • 14 years ago

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.

	Mike Hommey [:glandium] Assignee
	Comment 33 • 14 years ago

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.

	Mike Hommey [:glandium] Assignee
	Comment 34 • 14 years ago

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.

	Mike Hommey [:glandium] Assignee
	Comment 35 • 14 years ago

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).

	Mike Hommey [:glandium] Assignee
	Comment 36 • 14 years ago

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.

	Mike Hommey [:glandium] Assignee
	Comment 38 • 14 years ago

Defining global instances of classes with vtables unfortunately creates a static initializer. Using the Lazy initializer helps getting rid of them.

	Mike Hommey [:glandium] Assignee
	Comment 39 • 14 years ago

(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

	Mike Hommey [:glandium] Assignee
	Comment 40 • 14 years ago

(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.

	Mike Hommey [:glandium] Assignee
	Comment 41 • 14 years ago

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.

	Mike Hommey [:glandium] Assignee
	Comment 42 • 14 years ago

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).

	dwitte@gmail.com
	Comment 43 • 14 years ago

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.

	Mike Hommey [:glandium] Assignee
	Comment 44 • 14 years ago

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.

	Mike Hommey [:glandium] Assignee
	Comment 45 • 14 years ago

Updated to fit the new Lazy implementation

	Mike Hommey [:glandium] Assignee
	Comment 46 • 14 years ago

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...)

	Mike Hommey [:glandium] Assignee
	Comment 47 • 14 years ago

Comment on attachment 481799 [details] [diff] [review]
Use Lazy initialization for SMIL types

I'd like feedback on comment 46.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 48 • 14 years ago

Yes, I think it would make sense to use function tables instead.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 49 • 14 years ago

(Although it's kinda sad we have to jump through these hoops.)

	Mike Hommey [:glandium] Assignee
	Comment 50 • 14 years ago

(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).

	Mike Hommey [:glandium] Assignee
	Comment 51 • 14 years ago

Comment on attachment 481432 [details] [diff] [review]
Remove static xpcom modules list static initializer [checked-in]

http://hg.mozilla.org/mozilla-central/rev/73389f240593

	Mike Hommey [:glandium] Assignee
	Comment 52 • 14 years ago

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

	Mike Hommey [:glandium] Assignee
	Comment 53 • 14 years ago

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

	Mike Hommey [:glandium] Assignee
	Comment 54 • 14 years ago

Comment on attachment 481439 [details] [diff] [review]
Initialize nsIPresShell::gCaptureInfo without a constructor [checked-in]

http://hg.mozilla.org/mozilla-central/rev/df36c2dcf88f

	Mike Hommey [:glandium] Assignee
	Comment 55 • 14 years ago

Comment on attachment 481440 [details] [diff] [review]
Don't use a constructor for nsDOMWorkerPrincipal [checked-in]

http://hg.mozilla.org/mozilla-central/rev/0a9ee83b1093

	Benjamin Smedberg
	Comment 56 • 13 years ago

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.

	Luke Wagner [:luke]
	Comment 57 • 13 years ago

Can you use mozilla::Maybe (in mfbt/Util.h)?

	Mike Hommey [:glandium] Assignee
	Comment 58 • 13 years ago

(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.

	Benjamin Smedberg
	Comment 59 • 13 years ago

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

	Benjamin Smedberg
	Comment 60 • 13 years ago

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.

	Mike Hommey [:glandium] Assignee
	Comment 61 • 11 years ago

Comment on attachment 481799 [details] [diff] [review]
Use Lazy initialization for SMIL types

Obsoleted (and fixed by bug 903543)

	Mike Hommey [:glandium] Assignee
	Comment 62 • 11 years ago

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)

	Nathan Froyd [:froydnj]
	Comment 63 • 11 years ago

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.

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)
	Comment 64 • 11 years ago

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.