Provide table-driven QI mechanism

RESOLVED FIXED

Status

()

Core
XPCOM
P2
normal
RESOLVED FIXED
12 years ago
10 years ago

People

(Reporter: bsmedberg, Assigned: bsmedberg)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Assignee)

Description

12 years ago
Forked from bug 313207 - provide a table-driven QI mechanism which will reduce
codesize and probably also increase QI performance due to processor cache locality.
Benjamin, are you diving into this?  I had some thoughts, which might wait for
JS2 work to get implemented.  One is to make single-threaded isupports QI maps
be self-organizing.  Another is negative caching, to handle the frequent miss
testing problem.

I'm happy to do this as a later step on top of table-driven QI at least for the
simple cases, but I wanted to mention it now in case it seems worth doing now,
or working together on the design more.

/be
Previous attempt is in bug 23737.
(Assignee)

Comment 3

12 years ago
Created attachment 200781 [details] [diff] [review]
Static table-driven queryinterface, rev. 1

This uses a table-driven QI mechanism where a IID->offset table is statically constructed and passed to NS_TableDrivenQI for ordinary cases. Inheriting and aggregated versions are handled in the tail of the QueryInterface method.

This proves a significant codesize win on all platforms. The performance tests I was able to do are a little bit more ambiguous: linux was a definite win (close to 8% speedup on a contrived looping testcase), while win had a 0.5% slowdown (same contrived testcase). I think that the numbers are close enough to good that I'd like to try and land this and see what the tinderbox perf numbers say.

This patch is just the xpcom/ bits... the rest of my tree is littered with update NS_DEFINE_STATIC_IID_ACCESSOR fixups for pseudo-interfaces which I don't intend to attach unless you want to see them for some reason.
Attachment #200781 - Flags: review?(shaver)
Beauty.  I don't think synthetic loop torture-benchmarks are meaningful.  I'm all in favor of getting this into some testerboxes, or just into the trunk, and seeing what Tp, Txul, etc. are.

/be

Comment 5

12 years ago
Does placing NS_DEFINE_STATIC_IID_ACCESSOR in header files really work properly with all supported compilers+linkers?
(Assignee)

Comment 6

12 years ago
It works on MSVC.net2003/linux-gcc4.0/mac-gcc3.3

Comment 7

12 years ago
Do those linkers coalesce duplicated nsID instances?  What about vc6sp5?  What about GCC 2.95.2 (i.e., gcc w/o NS_HIDDEN)?
(Assignee)

Comment 8

12 years ago
Absolutely: C++ templates wouldn't make any sense unless they used weak/gnu-linkonce to coalesce symbols (the templatized hashtables would be enourmous!)
(Assignee)

Updated

12 years ago
Priority: -- → P2
In theory one could use some sort of hashing to avoid a loop. For example, if you know that there is some N-bit subfield of the IIDs for a class such that the subfield of each IID gives a unique value, then you can implement QI by extracting the bitfield, using it to index a table, and comparing the given IID with the IID in the table; if the IIDs don't match then you know the interface is not supported.

Unfortunately constructing such tables would require some serious magic.
gperf?
Computing what the subfield and IID table should be, given a list of IIDs, is easy; brute force would suffice. The magic part would be integrating that algorithm into the build.

Comment 12

12 years ago
I suppose this doesn't work, does it?

#define NS_DEFINE_STATIC_IID_ACCESSOR(the_interface, the_iid) \
NS_SPECIALIZE_TEMPLATE struct COMTypeInfo<the_interface> { \
static const nsIID kIID NS_HIDDEN; \
}; \
const nsIID COMTypeInfo<the_interface>::kIID NS_HIDDEN = the_iid;

#define NS_INTERFACE_TABLE_ENTRY(_class, _interface) \
  { &COMTypeInfo<_interface>::kIID, \
etc. etc.
(Assignee)

Comment 13

12 years ago
No, the NS_SPECIALIZE_TEMPLATE stuff uses strong symbols on windows instead of weak symbols, so you end up with multiply-defined-symbol errors :-(
Comment on attachment 200781 [details] [diff] [review]
Static table-driven queryinterface, rev. 1

This looks fine.  I have a slight preference for landing the FASTCALL changes first, to isolate strangeness and perf effects.

I trust we have build-size and perf numbers for all of the big 3, at least?

r=shaver
Attachment #200781 - Flags: review?(shaver) → review+
(Assignee)

Comment 15

12 years ago
The fastcall bits have landed. I'm going to land the NS_DECLARE/NS_DEFINE macro changes tomorrow morning with a closed tree... I'll change the templated NS_GET_IID on Monday morning and do the table-driven QI switch last, Monday afternoon or Tuesday.
(Assignee)

Comment 16

12 years ago
Part 2, which changes xpidl and the pseudo-interface declarations in the tree, has been checked in.
(Assignee)

Comment 17

12 years ago
Part 3 was checked in and then backed out, the old mac tboxen with xcode 1.1 were broken by the template changes, as well as MSVC6 which was having problems with ambiguous name resolution of multiply-inherited interfaces. I was able to fix MSVC6 pretty easily by making NS_GET_IID use a ::Interface, but the xcode stuff will have to wait until the tinderboxen are upgraded (we've already officially stopped supporting building with xcode1.1). Bug 299214 covers the tinderbox upgrade.
Depends on: 299214
(Assignee)

Comment 18

11 years ago
Part 3 checked in and backed out again, broke gcc2.9x and MSVC6 (different error this time, I think it has to do with "typedef class nsIFrame nsIBox;" in nsIFrame.h.

Comment 19

11 years ago
Could it be considered to support table-driven QI on known "good" compilers ?
(I'm thinking of VS 2005)
(Assignee)

Updated

11 years ago
Depends on: 327092
Revision 3.2 of xpcom/glue/pldhash.h, attributed to this bug, changed a generated file, pldhash.h, without changing the file from which it was generated, js/src/jsdhash.h.
(Assignee)

Comment 21

11 years ago
Fixed on trunk. I'll file followups to make in-tree code that currently uses the old-style interface maps use the new table-driven ones.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 22

11 years ago
I wonder if this broke something on BeOS (gcc 2.95.3, ld 2.15).

Short version (adding a longer output as patch):
When linking libtoolkitcomps.so
../../../dist/lib/liburlclassifier_s.a(nsUrlClassifierDBService.o)(.nsUrlClassifierDBService::gnu.linkonce.t.GetIID(void)+0x12): In function `nsUrlClassifierDBService::GetIID(void)':
/mozdev/dbg-ffox/toolkit/components/url-classifier/src/../../../../dist/include/xpcom/nsCOMPtr.h:467: undefined reference to `nsUrlClassifierDBService::COMTypeInfo<int>::kIID'

Comment 23

11 years ago
Created attachment 245846 [details]
Build Failure on BeOS
(Assignee)

Comment 24

11 years ago
gcc2.9x does not support the proper templated static members for this patch and is unsupported.

Comment 25

11 years ago
So basically the BeOS-platform is no longer supported at all?

Comment 26

11 years ago
well, be-os has never been officially supported. :-)

Comment 27

11 years ago
Well it was a port under Mozilla. (Just want to gather facts on what do next.)
I guess I should read up on the new minimum build requirements. Is there any other info I can find on this?

Comment 28

11 years ago
even with the smileyface that sounded pretty mean.  sorry.

I would think that for BeOS, you would try out gcc 3.4.3 or whatever the latest gcc for BeOS is.  I quickly read over the release notes and it doesn't sound like any of the bugs will block building ff.
Created attachment 245849 [details] [diff] [review]
patch to revert the third (last) checkin for this bug

Benjamin checked in the patch in three parts which aren't attached here as separate diffs. I already reverted part three while debugging another bug, so here it is in case anyone else finds it useful for testing.
(Assignee)

Comment 30

11 years ago
I've heard there is a version of BeOS using gcc3.x that we could still compile on. But yes, the trunk code is not going to work on the older BeOS. I don't know that we have a good place to document this other than perhaps http://developer.mozilla.org/en/docs/Linux_Build_Prerequisites#Build_Tools

Comment 31

11 years ago
BeOS has newer gcc's but they only work for C not C++ due to the ABI changes in gcc3. The kernel and all libs are in the old ABI.

Therefore we can no longer build trunk or supply Firefox for the existing BeOS. Haiku which is an upcoming opensource version will support gcc4, but I don't think it would be anywhere near stable to compile Firefox on, and it's netstack is not even finished.

So if gcc2.95.3 support is dropped it's pretty much end of the line for BeOS altogether with probably a new beginning 1/2-1year from now with Haiku. (http://haiku-os.org/)

Comment 32

11 years ago
BeOS successor Zeta OS promised gcc4-based version somewhere at 2.0, but now it is at 1.21, and gcc4-compiled version exists only on main developer harddisk.

But there is one half-optimistic detail. Zeta OS maintainer told once that they have something like ABI-wrapper for Zeta already, which allows to work gcc4-compiled C++/GUI programs to work on Zeta 1.21.

I'm wondering if he can publish those wrappers, aswell gcc4 itself, to use it for all BeOS version, but have deep doubts there, as he is for sure more interested in selling Zeta OS, not in keeping obsolete versions of OS alive.

Comment 33

11 years ago
(In reply to comment #24)
> gcc2.9x does not support the proper templated static members for this patch and
> is unsupported.
> 
Bug 361542 opened to document BeOS bustage and a place to document any possible workaround.

Updated

11 years ago
Depends on: 361917

Comment 34

11 years ago
It appears from 
https://bugzilla.mozilla.org/show_bug.cgi?id=361542#c11
that BeOS build wasn't fundamentally broken due gcc 2.953 legacy - taht old gcc is simply "too strict":) and bustage can be fixed with explicit correct definition.
Plugins and extensions that use the NS_IMPL_ISUPPORTS macros (and
build in the Mozilla source tree) may have a hard time with this:
Unless they can find a way to weakly link in NS_TableDrivenQI(),
they'll have to unroll their NS_IMPL_ISUPPORTS macros.

Have you considered maintaining versions of the NS_IMPL_ISUPPORTS
macros that don't use table-driven QI?

(I've already worked around the problem in my own plugin (the MRJ
Plugin JEP component of the Java Embedding Plugin for Mac OS X), and I
don't actually know if any other plugins or extensions use the
NS_IMPL_ISUPPORTS macros.  So the problem may not be urgent.  In fact
I neither unrolled my NS_IMPL_ISUPPORTS macros nor used weak linking:
OS X supports something called the "two-level namespace", which allows
multiple definitions of symbols, but which also does a very good job
of choosing the "right" one.)
(Assignee)

Comment 36

11 years ago
Steven, I'm not sure what weak linkage has to do with this. NS_TableDrivenQI is provided by the XPCOM glue static library and should be linked to there. It also happens to be exported from libxpcom_core.dylib for use by internal-linkage code, but code like JEP really shouldn't be using internal linkage.
The MRJCarbon plugin (and my MRJ Plugin JEP successor to it) has used
the NS_IMPL_ISUPPORTS for many years.  It used to be you could do this
without linking in any browser code.  Now, if you want to keep using
NS_IMPL_ISUPPORTS, you have to link in NS_TableDrivenQI().
(Assignee)

Comment 38

11 years ago
That's how the glue is designed, and it's required for all sorts of other uses, like nsCOMPtr. Link and be happy!
> Link and be happy!

My whole point is that linking gives you lots of reasons to be
unhappy.
(Assignee)

Comment 40

11 years ago
Was that your point? You haven't said *why* it makes you unhappy.
> You haven't said *why* it makes you unhappy.

Yes I did.  In comment #35.

But to restate my point succinctly:  Having to link in
NS_TableDrivenQI() makes things harder for writers of plugins and
extensions, because they have to worry about that symbol being
multiply defined -- once in the plugin/extension, and another time in
the browser.

As I also said in comment #35, I've found a workaround for the
problem, so _I'm_ not particularly unhappy.

I just wanted to point out something that you may have overlooked (and
apparently did overlook), which might impact the authors of other
plugins and extensions.
In the light of a new day, I realize I made a dumb mistake.  I mixed
up the following:

1) A host program loading a shared library when both define the same
   (public, external) symbol -- which can cause trouble.

2) A host program loading a shared library that staticly links to a
   (public) symbol defined in the host.  This means the aggregate will
   have two copies of whatever the symbol points to (and other
   dependent symbols it may drag in).  But this won't be a problem if
   the symbols point to code (i.e. if the dependent symbols don't
   include any global variables).

A shared library that uses NS_IMPL_ISUPPORTS (and staticly links in
NS_TableDrivenQI()) fits case #2.

I withdraw all my objections.  Sorry for the confusion :-(
> A host program loading a shared library

"Loading" should have been "dynamically loading".
Depends on: 387926
You need to log in before you can comment on or make changes to this bug.