1446509 - Create 'final' versions of AddRef and Release macros

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Bug 1332680 has identified a lot of Release and AddRef members on various classes that could be marked final and enable devirtualiation.  But those members are generated from macros, so we would need macro versions that have the final keyword.

Comment 1

[Nathan Froyd [:froydnj]]

Can the classes themselves not be marked `final`?  Or is this mostly applicable to classes like `Runnable`, which are intended to be subclassed, but AddRef/Release are basically not overridable by subclasses, so those methods should be marked `final`?

Comment 2

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

(In reply to Nathan Froyd [:froydnj] from comment #1)
> Can the classes themselves not be marked `final`?  

Only some can.

> Or is this mostly
> applicable to classes like `Runnable`, which are intended to be subclassed,
> but AddRef/Release are basically not overridable by subclasses, so those
> methods should be marked `final`?

Precisely.

Comment 3

[Nathan Froyd [:froydnj]]

NS_INLINE_DECL_THREADSAFE_REFCOUNTING, at least, supports passing method attributes as __VA_ARGS__, so we could easily pass `final` there.

Comment 4

[Alex Gaynor [:Alex_Gaynor]]

How hard would it be to add the same capability to NS_IMPL_CYCLE_COLLECTING_ADDREF and friends, which seem to be very common instances of this pattern?

Comment 5

[Nathan Froyd [:froydnj]]

Changing the macro is two lines of code:

#define NS_IMPL_CYCLE_COLLECTING_ADDREF(klass, ...) \
  NS_IMETHODIMPL_(MozExternalRefCountType) klass::AddRef(void) __VA_ARGS__ \
  ...

But maybe this particular macro, we can just say things are always `final`?  Andrew, how common/smart would it be to override AddRef/Release in cycle collected classes?

Comment 6

[Andrew McCreight [:mccr8]]

NS_DECL_ISUPPORTS_INHERITED is very common (this isn't only CCed classes, of course). I see 486 instances of it (including generated code). However, the actual implementation of AddRef and Release for classes that use this macro will be trivial. NS_IMPL_ADDREF/RELEASE_INHERITED are trivial wrappers to make refcount logging better, which is only used in NS_BUILD_REFCNT_LOGGING (aka debug builds). I've assumed that any whole program optimization we use would be smart enough to boil that off and devirtualize these methods but I've never checked it. Potentially they could be made final in opt builds but not in debug builds, though that feels sketchy.

Comment 7

[Nathan Froyd [:froydnj]]

I'd be in favor of somehow making NS_IMPL_ADDREF/RELASE_INHERITED be essentially no-ops in opt builds: slightly less code to drag around because we're not generating methods that are just forwarding methods.  That'd require a bit of care of NS_DECL_ISUPPORTS_INHERITED, though...

I think it'd be a pretty small win, though, as far as code size goes.  Slightly better icache performance?

Comment 8

[Alex Gaynor [:Alex_Gaynor]]

Depends on if the compiler/linker is really managing to devirtualize them or not. If they're not getting devirtualized it's potentially a more sizable win.

Comment 9

[Andrew McCreight [:mccr8]]

I should be more precise and say that AddRef and Release still have to be virtual methods in general, but specific call sites that know about any concrete class should potentially be able to make non-virtual calls to AddRef and Release. (Well, I'm not sure what happens with multiple inheritance).

Comment 10

[Alex Gaynor [:Alex_Gaynor]]

Attachment: Bug 1446509 - added final versions of macro for declaring AddRef and Decref; r?froydnj

Also make use of them in a few places.

Comment 11

[Phabricator Automation]

Comment on attachment 8997112 [details]
Bug 1446509 - added final versions of macro for declaring AddRef and Decref; r?froydnj

Nathan Froyd [:froydnj] has approved the revision.

https://phabricator.services.mozilla.com/D2678

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8a23e3023c92
added final versions of macro for declaring AddRef and Decref; r=froydnj

Comment 13

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/8a23e3023c92