49416 - Modify xpidl to emit __declspec for space savings on windows

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement, P3)

Description

[Mike McCabe]

Adam Lock posts on .xpcom:

I've attached a patch below which enables the use of VisualStudio's
__declspec(novtable) macro on Win32 builds. The patch contains a small
modification to the IDL compiler and a new macro NS_DECL_INTERFACE in
nsISupportsUtils.h. The __declspec command tells VC++ to omit
constructor vtable code when it is unnecessary such as with pure virtual
classes. Other platforms get built the old way.

The __declspec(novtable) command is described here:

http://msdn.microsoft.com/library/devprods/vs6/visualc/vccore/_langref_novtable.htm

This results in size savings of up to 10% on some interface intensive
DLLs such as gkthml.dll. Greater benefits would be felt if the C++ &
idlc interfaces also used this macro. I haven't measured what effect
this has on performance or memory consumption.

Does gcc provide a similar command to disable vtable generation code
when it isn't necessary?

Adam

Comment 1

[Mike McCabe]

Attachment: Patch to xpidl and nsISupportsUtils.h

Comment 2

[Mike McCabe]

Adam, do you have any measurements as to how much space this saves in practice?

Mike

Comment 3

[Mike McCabe]

jband made a suggestion to make this a little safer -

It's possible for this optimization to be unsafe whenever there are constructor
methods in the interface that call virtual functions.  With plain IDL, this
can't happen; however, bad code might be added to interfaces with %{ %} blocks -
to keep this optimization from being a potential boobytrap, we could only emit
the declaration when there are no included %{ blocks.

Comment 4

[Adam Lock]

Interface constructors are illegal, so why should we be trying to accomodate 
people who abuse %{ %} blocks to include them?

Comment 5

[Adam Lock]

Oops I spoke too soon. My size claim is totally bogus.

I've built Mozilla with and without the fix and there appears to be no 
appreciable difference in file size.

Previously I had been comparing figures between my local build with the nightly 
build (with has the vtables) and I was seeing the file difference. I suppose 
something like FullCircle may have been the cause of the 10% difference though I 
downloaded the version that said it didn't have it.

Even so, it could still be worth applying this patch since it may marginally 
improve class construction times.

Comment 6

[Adam Lock]

I've done a build with and without the patch. Most files stay the same size, but 
a few differ:

File             vtable     No vtable
msgbsutl.dll     139264     135168
xpcom.dll        352256     348160
absyncsv.dll     49152      45056
addrbook.dll     155648     147456
emitter.dll      40960      36864
gkhtml.dll       1630208    1626112
msgcompo.dll     184320     180224
msgdb.dll        61440      57344
msgimap.dll      249856     245760
xppref32.dll     36864      32768

Interestingly, all files seem to be rounded up to multiples of 4096 bytes in 
length. I think that the novtable patch reduces all DLLs a little bit but only 
some reduce down to the next 4096 multiple.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

You're talking about optimized builds, not debug, right?

Comment 8

[Adam Lock]

Yes, I'm talking about optimized builds here.

Comment 9

[John Bandhauer]

Attachment: proposed patch

Comment 10

[John Bandhauer]

I just attached a patch that I like better for two reasons.

1) I **really** prefer the form...
  class NS_NO_VTABLE nsIFoo
...over the form...
  NS_DECL_INTERFACE(nsIFoo)

I don't see a reason to mangle a class declaration that much. If we get a reason 
then we can do such mangling then. Note that our friends at MS use the less 
mangled form.

2) Adam's original patch did not set NS_NO_VTABLE on nsISupports! This is 
likely to miss out on roughly half of whatever *time* savings there may be by 
avoiding the vtbl* settings during object construction.

Another point... By introducing this into our code we are going to have to watch 
out for good intentioned people making the mistake of copying this into the 
declarations of non-abstract classes.

Nevertheless I think that this should *for sure* go in.

Comment 11

[Mike McCabe]

Marking non-confidential.

I see 45k space savings... not nothing but not much.  If it's a big time
savings, it will be worth it.  nsbeta3?

Comment 12

[Mike McCabe]

John - I have modifications to xpidl ready to detect the
%{-escape-within-interface case.  Are they needed?  Could you remind me (and
Adam) why for the record?

Comment 13

[Adam Lock]

The NS_DECL_INTERFACE macro is there for pure laziness reasons - if we add more 
compiler hints in future we don't have to change the xpidl to incorporate them - 
just nsISupportsUtils.h needs changing.

I'm happy with either way though.

Comment 14

[Mike McCabe]

Nominating nsbeta3 based on jband comments -

John, can you corraborate / push process?

Comment 15

[Patrick C. Beard]

Updating for checkin.

Comment 16

[Mike McCabe]

Attachment: detect whether idl contains %{ before emitting __declspec

Comment 17

[Mike McCabe]

Attachment: Another stab at it, updated for new macro name.

Comment 18

[Mike McCabe]

Checked in.

Comment 19

[Mike McCabe]

Attachment: changes to nsISupportsUtils.h (may not be in sync with others.)

Comment 20

[Mike McCabe]

Cleaning out an older tree, found this diff to save off.  I didn't notice that
the fix had already been checked in, though!