1293870 - Stop generating NS_DECL_NON_VIRTUAL_NSI* macros?

Status: RESOLVED WONTFIX

(Core :: XPCOM, defect, P3)

Description

[Nicholas Nethercote [inactive]]

For every IDL class we generate two macros like this:

> /* Use this macro when declaring classes that implement this interface. */
> #define NS_DECL_NSISUBSTITUTINGPROTOCOLHANDLER \
>   NS_IMETHOD SetSubstitution(const nsACString & root, nsIURI *baseURI) override; \
>   NS_IMETHOD GetSubstitution(const nsACString & root, nsIURI * *_retval) override; \
>   NS_IMETHOD HasSubstitution(const nsACString & root, bool *_retval) override; \
>   NS_IMETHOD ResolveURI(nsIURI *resURI, nsACString & _retval) override;
> 
> /* Use this macro when declaring the members of this interface when the
>    class doesn't implement the interface. This is useful for forwarding. */
> #define NS_DECL_NON_VIRTUAL_NSISUBSTITUTINGPROTOCOLHANDLER \
>   NS_METHOD SetSubstitution(const nsACString & root, nsIURI *baseURI); \
>   NS_METHOD GetSubstitution(const nsACString & root, nsIURI * *_retval); \
>   NS_METHOD HasSubstitution(const nsACString & root, bool *_retval); \
>   NS_METHOD ResolveURI(nsIURI *resURI, nsACString & _retval);

Only two of the 1500+ NON_VIRTUAL ones get used -- NS_DECL_NON_VIRTUAL_NSIPROTOCOLHANDLER and NS_DECL_NON_VIRTUAL_NSISUBSTITUTINGPROTOCOLHANDLER -- both in netwerk/protocol/res/SubstitutingProtocolHandler.h. (I checked comm-central as well -- no additional NON_VIRTUAL uses there.)

If we stop generating these macros and write by hand the two ones that are required, on my Linux64 machine it reduces the amount of code in $OBJDIR/dist/include by 5%, which can only help build times.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Stop generating NS_DECL_NON_VIRTUAL_NSI* macros

Nathan, what do you think about this? I'm not strongly wedded to it. It's just
something that caught my eye while I was looking at NS_IMETHOD/NS_METHOD stuff,
because I'm wondering whether NS_METHOD can be removed.

Comment 2

[Nicholas Nethercote [inactive]]

Oh, now I see that bholley introduced this in https://hg.mozilla.org/mozilla-central/rev/f659ec819bf6. The commit message explains why NS_IMETHOD is avoided:

> Generate an extra macro to declare a non-virtual variant of an interface. r=billm
> 
> This allows us to have a shared superclass that implements the guts of a shared
> superinterface, without having the superclass actually inherit the superinterface
> (which leads to annoying and unnecessary diamond-inheritance). 

(I half-understand that.)

One question is whether it needs to be NS_METHOD (which adds __stdcall on Windows) or whether a vanilla nsresult return type would suffice. I suspect nsresult would suffice.

Comment 3

[Nicholas Nethercote [inactive]]

So, to clarify, there are two independent questions here:

- Can we avoid generating 1000s of unused macros?

- Can we avoid using NS_METHOD in these macros?

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to Nicholas Nethercote [:njn] from comment #2)
> One question is whether it needs to be NS_METHOD (which adds __stdcall on
> Windows) or whether a vanilla nsresult return type would suffice. I suspect
> nsresult would suffice.

It would probably suffice, but I think __stdcall produces smaller code, as the cleanup code for the stack lives in the callee (1 place) rather than the caller (potentially many places).

(In reply to Nicholas Nethercote [:njn] from comment #3)
> So, to clarify, there are two independent questions here:
> 
> - Can we avoid generating 1000s of unused macros?

Yes.  We can do it how you have done it, or we could generate something like:

#define NS_DECL_IFOO_GUTS(qualifier) \
  qualifier Method1(...) = 0; \
  qualifier Method2(...) = 0; \
  ...

#define NS_DECL_IFOO NS_DECL_IFOO_GUTS(NS_IMETHOD)
#define NS_DECL_NON_VIRTUAL_IFOO NS_DECL_IFOO_GUTS(NS_METHOD)

(I think that expands to the right thing?  Might need a bit more macro hackery?)?  That would at least make the instantiation of the *NON_VIRTUAL* macros lazy...  I like the second way a little better, as the maintenance burden is less.

We could also do it with an [include_non_virtual_interface_macros] on the interface, but that feels a bit clunky.

I must say I've very surprised by how much code this shaves off!  Maybe we should investigate ways to trim down the headers even more...?

> - Can we avoid using NS_METHOD in these macros?

I think we should avoid doing this for the space reasons mentioned above.

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8779591 [details] [diff] [review]
Stop generating NS_DECL_NON_VIRTUAL_NSI* macros

Review of attachment 8779591 [details] [diff] [review]:
-----------------------------------------------------------------

Please see comment 4.

Comment 6

[Bobby Holley (:bholley)]

NS_DECL_IFOO_GUTS sounds good to me. I think I tried to do something like that but the XPIDL compiler made it hard for some reason? Hard to remember.

Comment 7

[Nicholas Nethercote [inactive]]

> It would probably suffice, but I think __stdcall produces smaller code, as
> the cleanup code for the stack lives in the callee (1 place) rather than the
> caller (potentially many places).

The number of NS_METHOD methods is < 200 in the whole codebase. (Even accounting for all the codegen'd NS_METHOD instances, because so few of those macros are actually used.) So I doubt it'll matter, but I'm happy to measure binary size if/when I try removing the __stdcall.