772987 - Add Static{Auto,Ref}Ptr, like ns{Auto,Ref}Ptr, but without the static initializer

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Justin Lebar (not reading bugmail)]

It's silly that, when we want static variables, we have to choose between startup speed (raw pointers) and safety (using static ns{Auto,Ref}Ptr, which creates an additional static initializer).

I have a patch to add clones of ns{Auto,Ref}Ptr which don't add static initializers.

Comment 1

[Justin Lebar (not reading bugmail)]

Attachment: Part 1: Add Static{Auto,Ref}Ptr.

Comment 2

[Justin Lebar (not reading bugmail)]

Attachment: Part 2: Use Static{Auto,Ref}Ptr where appropriate.

This is mostly hal code, so over to Mounir.

Comment 3

[Justin Lebar (not reading bugmail)]

Actually, I should probably hold off on the reviews until after I get try results back, because I have a hunch something isn't going to compile somewhere.

Comment 4

[Justin Lebar (not reading bugmail)]

https://tbpl.mozilla.org/?tree=Try&rev=5fc9ae51b2fe

Comment 5

[Justin Lebar (not reading bugmail)]

Attachment: Part 3: Update comment in ClearOnShutdown.

Comment 6

[(no longer active)]

Comment on attachment 641158 [details] [diff] [review]
Part 1: Add Static{Auto,Ref}Ptr.

Can you please file a static analysis bug to make us actually check that these classes are only used in the global scope?  I wouldn't trust people with this kind of power even for a second!  ;-)

Comment 7

[Justin Lebar (not reading bugmail)]

> I wouldn't trust people with this kind of power even for a second!  ;-)

There's a dynamic (debug-time) assertion that mRawPtr is 0, during the constructor.  So hopefully that will catch some cases.

Static analysis would be great, though.

Comment 8

[(no longer active)]

(In reply to comment #7)
> > I wouldn't trust people with this kind of power even for a second!  ;-)
> 
> There's a dynamic (debug-time) assertion that mRawPtr is 0, during the
> constructor.  So hopefully that will catch some cases.

Ah, hmm, that should take care of a whole bunch of cases, yes.  But I mean, once we get NS_STACK_CLASS working (which is close if I understand things correctly), this should be easy.  :-)

Comment 9

[Justin Lebar (not reading bugmail)]

Comment on attachment 641158 [details] [diff] [review]
Part 1: Add Static{Auto,Ref}Ptr.

try is pretty horked, with no recovery in sight, so I guess I'll just use m-i as my tryserver.

Comment 10

[Justin Lebar (not reading bugmail)]

Comment on attachment 641165 [details] [diff] [review]
Part 3: Update comment in ClearOnShutdown.

I'd normally ask Waldo to review this comment change, but he's busy.

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

You might be able to ~export

http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsTraceRefcntImpl.cpp#133

and use it to assert that these helpers are *only* constructed/destructed from ~static contexts.

Comment 12

[Justin Lebar (not reading bugmail)]

Seems like that would only work in builds with --enable-trace-malloc (which isn't the default with debug builds?  Or is it?).  In any case, I could certainly do something similar.  The trick is hearing that we're starting up very early in the game, and hearing that we're shutting down very late in the game.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Wouldn't it be less maintenance burden to make these base classes of nsAutoPtr / nsRefPtr and move all methods except for the destructor to them?  Then there'd be no duplication of code, they'd provide the same API, and any future additions would automatically apply to both.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

er, destructor and constructor.  (You'd also need to add a second protected constructor with a funky signature for the derived class to call, to avoid the assert.)

Comment 15

[Justin Lebar (not reading bugmail)]

(In reply to David Baron [:dbaron] from comment #13)
> Wouldn't it be less maintenance burden to make these base classes of
> nsAutoPtr / nsRefPtr and move all methods except for the destructor to them?
> Then there'd be no duplication of code, they'd provide the same API, and any
> future additions would automatically apply to both.

I considered that, but I rejected it because I'd still have to duplicate a bunch of operators on nsAutoPtr which return nsAutoPtr&, and fix similar problems.  (Maybe there's a way around these using C++-fu that I do not yet have.)

I'm not really bothered that you can't do getter_AddRefs() on a StaticRefPtr, personally.

Comment 16

[Justin Lebar (not reading bugmail)]

https://tbpl.mozilla.org/?tree=Try&rev=9a78335b8413

Comment 17

[Justin Lebar (not reading bugmail)]

(In reply to Justin Lebar [:jlebar] (PTO July 26) from comment #16)
> https://tbpl.mozilla.org/?tree=Try&rev=9a78335b8413

Actually, scratch that; I already had a basically-working test run in comment 4.

http://hg.mozilla.org/integration/mozilla-inbound/rev/b9a1f5cae6ea
http://hg.mozilla.org/integration/mozilla-inbound/rev/d5bc3b732229
http://hg.mozilla.org/integration/mozilla-inbound/rev/7517b13c4d57

Comment 18

[Mozilla RelEng Bot]

Try run for 9a78335b8413 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=9a78335b8413
Results (out of 17 total builds):
    exception: 17
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-9a78335b8413

Comment 19

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

Maybe this should have gone into mfbt...

Comment 20

[Justin Lebar (not reading bugmail)]

Well, the mfbt RefPtr is not compatible with XPCOM objects...

Comment 21

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/b9a1f5cae6ea
https://hg.mozilla.org/mozilla-central/rev/d5bc3b732229
https://hg.mozilla.org/mozilla-central/rev/7517b13c4d57

Comment 22

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

(In reply to Justin Lebar [:jlebar] from comment #20)
> Well, the mfbt RefPtr is not compatible with XPCOM objects...

That's news to me... why?

Comment 23

[Justin Lebar (not reading bugmail)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #22)
> (In reply to Justin Lebar [:jlebar] from comment #20)
> > Well, the mfbt RefPtr is not compatible with XPCOM objects...
> 
> That's news to me... why?

Actually, I'm totally incorrect.

Comment 24

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Yes, in fact, thanks to Bas it was made MSCOM-compatible at the cost of some unsafety.

Comment 25

[Jesse Ruderman]

I'd expect compilers to eliminate static initializers that just set members to zero.  Was that not happening?

Comment 26

[Nathan Froyd [:froydnj]]

(In reply to Jesse Ruderman from comment #25)
> I'd expect compilers to eliminate static initializers that just set members
> to zero.  Was that not happening?

GCC, at least, does not do this.  (Long-standing PR.)

Comment 27

[Justin Lebar (not reading bugmail)]

There's also the problem of non-trivial static /destructors/, which nsRefPtr does have.  And I vaguely recall something awful like the presence of a non-trivial static destructor requires the presence of a static constructor.

Comment 28

[Mike Hommey [:glandium]]

(In reply to Justin Lebar [:jlebar] from comment #27)
> There's also the problem of non-trivial static /destructors/, which nsRefPtr
> does have.  And I vaguely recall something awful like the presence of a
> non-trivial static destructor requires the presence of a static constructor.

Indeed, static destructors are registered with __cxa_atexit in a static constructor. Note that this one applies to MSVC too, contrary to the other case mentioned.