877850 - get rid of some static constructors in xpcom/

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Comment 1

[Trevor Saunders (:tbsaunde)]

Attachment: bug 877850 - fix static constructors in xpcom/

Comment 2

[Justin Lebar (not reading bugmail)]

I'm not an expert on C++ arcanea, but I'm worried about a few things here.

* If it's not too hard, can we check that this change actually decreases the
  number of static constructors?  The reason I ask is, I think this still
  leaves us with static destructors (1), and I don't know whether our compilers
  can implement static destructors without a corresponding constructor.
  
  We certainly don't want to promote this idiom if it doesn't actually fix the
  problem it's trying to fix.

* In C++11, this idiom requires the compiler to check for the static variable's
  initialization in a threadsafe matter (2).  I'd hope that this is fast and
  low-overhead, but I don't know whether that's the case.

http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3337.pdf

(1) Section 6.7.5: "The destructor for a block-scope object with static or
thread storage duration will be executed if and only if it was constructed." 

(2) Section 6.7.4: "An implementation is permitted to perform early
initialization of other block-scope variables with static or thread storage
duration under the same conditions that an implementation is permitted to
statically initialize a variable with static or thread storage duration in
namespace scope (3.6.2).  [I don't think we're in this case.] Otherwise such a
variable is initialized the first time control passes through its declaration;
such a variable is considered initialized upon the completion of its
initialization."

Comment 3

[Justin Lebar (not reading bugmail)]

Comment on attachment 756204 [details] [diff] [review]
bug 877850 - fix static constructors in xpcom/

I defer to Jeff, who actually knows C++.

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to Justin Lebar [:jlebar] from comment #2)
> * If it's not too hard, can we check that this change actually decreases the
>   number of static constructors?  The reason I ask is, I think this still
>   leaves us with static destructors (1), and I don't know whether our
> compilers
>   can implement static destructors without a corresponding constructor.

The compilers can implement this.  What happens when the function-level static variable is constructed is that the equivalent of an atexit() handler is registered that performs the destruction.  The same handler registration happens with static constructors, too.

> * In C++11, this idiom requires the compiler to check for the static
> variable's
>   initialization in a threadsafe matter (2).  I'd hope that this is fast and
>   low-overhead, but I don't know whether that's the case

It's reasonably low-overhead; in GCC and clang the determination of whether the variable has been constructed is implemented as a test-and-set of a boolean flag.  I haven't looked at MSVC, but I assume it implements things in a similar manner.  (It's possible, of course, that the compiler might turn the constructor into constant data.)

Comment 5

[Trevor Saunders (:tbsaunde)]

only replying to this part since froydnj already got the rest before I got there

> * If it's not too hard, can we check that this change actually decreases the
>   number of static constructors?  The reason I ask is, I think this still

yeah, I checked before posting nm libxul.so | grep GLOBAL__sub_I_<file> for those files gives nothing and did before.

Comment 6

[Justin Lebar (not reading bugmail)]

Comment on attachment 756204 [details] [diff] [review]
bug 877850 - fix static constructors in xpcom/

Okay!

>     static EmptyEnumeratorImpl* GetInstance() {
>+      static const EmptyEnumeratorImpl kInstance;
>         return const_cast<EmptyEnumeratorImpl*>(&kInstance);
>     }

Indentation

> nsSimpleUnicharStreamFactory*
> nsSimpleUnicharStreamFactory::GetInstance()
> {
>+  static const nsSimpleUnicharStreamFactory kInstance;
>   return const_cast<nsSimpleUnicharStreamFactory*>(&kInstance);
> }

The const_cast here and above are pretty weird, but I guess we can leave them as they were.

Comment 7

[Trevor Saunders (:tbsaunde)]

> > nsSimpleUnicharStreamFactory*
> > nsSimpleUnicharStreamFactory::GetInstance()
> > {
> >+  static const nsSimpleUnicharStreamFactory kInstance;
> >   return const_cast<nsSimpleUnicharStreamFactory*>(&kInstance);
> > }
> 
> The const_cast here and above are pretty weird, but I guess we can leave
> them as they were.

I thought the same but figured better to worry about that some other time.

Comment 8

[Trevor Saunders (:tbsaunde)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d96df10fe4f5

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d96df10fe4f5