Closed Bug 49416 Opened 24 years ago Closed 24 years ago

Modify xpidl to emit __declspec for space savings on windows

Categories

(Core :: XPCOM, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED

People

(Reporter: mike+mozilla, Assigned: mike+mozilla)

Details

(Whiteboard: [nsbeta3+])

Attachments

(5 files)

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
Adam, do you have any measurements as to how much space this saves in practice? Mike
Status: NEW → ASSIGNED
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.
Interface constructors are illegal, so why should we be trying to accomodate people who abuse %{ %} blocks to include them?
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.
Group: netscapeconfidential?
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.
You're talking about optimized builds, not debug, right?
Yes, I'm talking about optimized builds here.
Attached patch proposed patch (deleted) — Splinter Review
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.
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?
Group: netscapeconfidential?
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?
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.
Nominating nsbeta3 based on jband comments - John, can you corraborate / push process?
Keywords: nsbeta3
Updating for checkin.
Whiteboard: [nsbeta3+]
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Cleaning out an older tree, found this diff to save off. I didn't notice that the fix had already been checked in, though!
Component: xpidl → XPCOM
QA Contact: mike+mozilla → xpcom
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: