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)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
People
(Reporter: mike+mozilla, Assigned: mike+mozilla)
Details
(Whiteboard: [nsbeta3+])
Attachments
(5 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•24 years ago
|
||
Assignee | ||
Comment 2•24 years ago
|
||
Adam, do you have any measurements as to how much space this saves in practice?
Mike
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•24 years ago
|
||
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.
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•24 years ago
|
||
You're talking about optimized builds, not debug, right?
Comment 9•24 years ago
|
||
Comment 10•24 years ago
|
||
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.
Assignee | ||
Comment 11•24 years ago
|
||
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?
Assignee | ||
Comment 12•24 years ago
|
||
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•24 years ago
|
||
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.
Assignee | ||
Comment 14•24 years ago
|
||
Nominating nsbeta3 based on jband comments -
John, can you corraborate / push process?
Keywords: nsbeta3
Assignee | ||
Comment 16•24 years ago
|
||
Assignee | ||
Comment 17•24 years ago
|
||
Assignee | ||
Comment 18•24 years ago
|
||
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 19•24 years ago
|
||
Assignee | ||
Comment 20•24 years ago
|
||
Cleaning out an older tree, found this diff to save off. I didn't notice that
the fix had already been checked in, though!
You need to log in
before you can comment on or make changes to this bug.
Description
•