108233 - Make generic module support usable by component and plugin developers

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Doug Turner (:dougt)]

I am worried that component and plugin developers will want to use this great
way to bootstrap a component library.  We should make it publically avaliable by
moving it to the xpcom/glue library and ensuring that it does not use any
private xpcom APIs.

Comment 1

[Alec Flett]

Yes, I'm a big supporter of making this available in the static helper library.

Comment 2

[John Bandhauer]

I think this would be a good thing too - isn't this the kind of thing the glue
lib is all about? Doug, you were expressing misgivings; have you figured out the
extent of the scope of this? What freezes and changes are required? Is there
something in particular that makes this look too painful?

Comment 3

[Doug Turner (:dougt)]

I think that this can be done without to much fuss.

Comment 4

[Doug Turner (:dougt)]

The only dependancy that I have identified which blocks this from being a simple
move out into the glue library is usage of a nsSupportsHashtable.  Maybe we can
use a nspr hashtable!  

Comment 5

[Alec Flett]

wait, but are people currently required to link against NSPR to get the glue?

This is potentially ugly...the hashtable is used to cache a factory for each
object. Damn. I wonder how many of those cached factories we have lying around.

Comment 6

[Doug Turner (:dougt)]

> wait, but are people currently required to link against NSPR to get the glue?

Nope. They will if we make this change... :-(

>This is potentially ugly...the hashtable is used to cache a factory for each
object. Damn. I wonder how many of those cached factories we have lying around.

All of them. :-/

Comment 7

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

We have all of them floating around, I suspect: the component manager caches any
factory it ever sees, IIRC, without any dropping of them at all.

PLDHash is in xpcom/ds, not libnspr, but I doubt we want it in the glue library.
 Can the glue version just use a linked list?  If a component has a large number
of factories to manage, and the author cares a lot about performance of getting
a factory (rare, given the aforementioned caching story), they can write a
custom nsIModule implementation using whatever hash or tree implementation they
want.

(They could even use the STL!)

Comment 8

[Doug Turner (:dougt)]

Mike, I was thinking about using plhash in nspr, but as Alec pointed out, that
would just suck to add a nspr dependancy.

I will hack up a version of the generic module code which uses a linked list and
get some performance/bloat numbers on it.  Stay tuned.

Comment 9

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

As I said above, pldhash isn't in NSPR, it's in xpcom/ds.  But again, you don't
want to link against libnspr.  I'm sure the linked list will be fine, and if
it's not they don't have to use our convenience class.

Comment 10

[Doug Turner (:dougt)]

plhash (note the missing letter) in xpcom, when did that happen?

Comment 11

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

plhash is in NSPR, and has been for ages.  I thought your comment that you were
going to use plhash was meant to be in reply to my comment that PLDHash was in
XPCOM.  None of it matters.

Comment 12

[Doug Turner (:dougt)]

Attachment: Uses a linked listed instead of nsSupportsHashtable

Comment 13

[Doug Turner (:dougt)]

The maximum walk is 38.  Average walk is 5.1, Median walk is 2.5.  We could
restructure a few of the nsModuleComponentInfo to reduce these numbers.
  
The allocation size - 3 pointers.  One for the factory, one for the cid, one for
the next element.

Overall, I do not see any startup different on my windows p3 1ghz.

I was thinking about just removing all of these factories and just handing over
ownership to the component manager, but that is a bad thing for a few reasons. 
Most notably, if we do hand over ownership, there will be now way to unload a
module as the module will never know what factories are active.  Not like this
is a concern now, but I don't want to have to rewrite this code later.

One other thing that just SUCKS.  The nsIDKey only does a 32 hash code.  If two
IID share the first 32 bits, they will collide.  See bug 127805.  This patch
avoids using this key.

Comment 14

[Doug Turner (:dougt)]

maximum walk = the largest number of factories in the modules I built (windows
standard gmake build).  This if, of course, unbounded; You could have n number
of factories a module.

Comment 15

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Why would we use a linked list for Mozilla components that already link against
libxpcom (most of them)?  We have access to a wide variety of hash tables, some
of them quite good.  For a module with 31 factories, it seems like that's what
we'd want.

Don't worry about factory and module unloading yet: if you're not going to do
the work to count instances, nsGenericModule::canUnload must always return
false.  This module code is going to get statically pulled into a pile of
components, and they _must_not_ lie to us.  So let's save the storage overhead
completely, and just hand them off to the component manager.  Whee!

(That may also be the case for components that are "internal" to Mozilla: are we
really going to flip the switch on unloadable components for 1.0?  I fear we'd
need another interface, nsIUnloadableModule, since there are probably piles of
components out there that mis-report canUnload, *sigh*.)

Comment 16

[Doug Turner (:dougt)]

> Why would we use a linked list for Mozilla components that already link against
libxpcom (most of them)?  We have access to a wide variety of hash tables, some
of them quite good.  For a module with 31 factories, it seems like that's what
we'd want.  

I wanted to have only one "generic module" implementation.  I could have two
code pathes depending on if the library is linked into xpcom proper.  In this
way, the glue could use a hashtable.  But I didn't see any real perf problems
with the above patch.  note that there is only 3 modules with that many
factories.  The median is extremely low.  Use of a hashtable would probably be
an overkill for the 80+% case.  

>Don't worry about factory and module unloading yet: if you're not going to do
the work to count instances, nsGenericModule::canUnload must always return
false.  This module code is going to get statically pulled into a pile of
components, and they _must_not_ lie to us.  So let's save the storage overhead
completely, and just hand them off to the component manager.  Whee!

I do have a patch which can unload a module if there are no outstanding
instances.  Having the module know about its factories is a requirement of
unloading.  If we want to give up on unloading for the generic module, we can go
down this road.  I don't agree with this path, but it would immediately solve
the problem about moving this code into the glue. 

>(That may also be the case for components that are "internal" to Mozilla: are we
really going to flip the switch on unloadable components for 1.0?  I fear we'd
need another interface, nsIUnloadableModule, since there are probably piles of
components out there that mis-report canUnload, *sigh*.)

Unloading will not happen for mozilla 1.0.  But my patch did not have to change
any exists frozen interfaces.  It modified the nsIGenericModule.idl, and it
modified the nsIComponentManagerObsolete (the functionality can be moved in to
another interface)

Comment 17

[Doug Turner (:dougt)]

Attachment: Removes the factory cache from the generic module code

size win.

Comment 18

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

How does changing those interfaces cope with the likely case that there are
modules out there that have their own nsIModule implementation and lie about
canUnload?  Didn't we hit that problem back when dp tried to flip that switch?

I'm not motivated by the fear of future change, where it costs us complexity,
space and speed now.  When we get a module unloading story we all like, we'll
rev some interface and write some code with a clearer picture than we have now.
 Let's not borrow expensive trouble.

(As I wrote this, dougt and I settled our differences in IRC, I think.)

Comment 19

[Doug Turner (:dougt)]

>How does changing those interfaces cope with the likely case that there are
modules out there that have their own nsIModule implementation and lie about
canUnload?  Didn't we hit that problem back when dp tried to flip that switch?

If they lie, then the whole system will go down if we do unloading.  I was
thinking about adding a new nsIClassInfo flag which indicates that a module
really wants to be unloaded.  There is a case to be made where a module "can" be
unloaded, but does not "want" to be unloaded.  This flag would also help us out
with rouge nsIModule implementations as only nsIModules with both the flag set
and canUnload==1, will be unloaded.  More thought is needed, but we can take it
off line.

>(As I wrote this, dougt and I settled our differences in IRC, I think.)

Yeah.  Get the size win now.  we don't really have a defined time when unloading
will actually work.  

John - please glance at the patch above.  I am not passing the factories which
are eager to the component manager.  I think this is okay, but I want your input.

Comment 20

[John Bandhauer]

So you're giving up on much of the point of the nsIModule abstraction?
nsGenericModule::GetClassObject no longer returns a singleton? Anyone who tries
to use it rather than going through the component manager is screwed. No? This
seems whacked to me.

Comment 21

[Doug Turner (:dougt)]

The nsIModule interface is only know by the component manager.  Clients should
*not* try to obtain one of these objects and using it directly.  If clients want
to get a class object, they should be using nsIComponentManager::GetClasObject.
 Am I missing something?

Comment 22

[Alec Flett]

Comment on attachment 71531 [details] [diff] [review]
Removes the factory cache from the generic module code

so dougt explained to me how XPCOM itself has its own cache of factories, so
removing this second level of caching should have no affect, aside from the
obvious linkage and space win.
sr=alecf

Comment 23

[Suresh Duddi (gone)]

Comment on attachment 71531 [details] [diff] [review]
Removes the factory cache from the generic module code

r=dp

Comment 24

[John Bandhauer]

I retrack my earlier objection - Doug is right, we enforce the idea that the
module is not exposed except via the component manager.

Still, Doug is going to post a patch with a small update. The case where the
constructor is null is used for classes that we want to have a factory (because
it is the object that implements nsIClassInfo) but where we don't want the
factory registered with the component manager. We do this in various cases so
that instances can expose nsIClassInfo without having registered factories and
without actually constructing them via the factory (even internal to the
module). The current patch would make these factories get registered. We'd like
to avoid that. The change that Doug and I discussed is to use a local linked
list of *those* factories and register the normal ones (when the 'eager' flag is
set as in the current code). The linked list would be iterated and the factories
released at module shutdown.

Also, Is it true that the RegisterFactory we'll be doing for those classes
marked EAGER_CLASSINFO will have no persistent effect on the
RegisterFactoryLocation registration done for those same classes in
RegisterSelf. I mean, the fact that we eagerly build and register those
factories at module init on one run of the app will not keep us from finding the
persistent registration and loading the module on demand on the next run, right?

Comment 25

[Doug Turner (:dougt)]

Comment on attachment 71531 [details] [diff] [review]
Removes the factory cache from the generic module code

this is not quite right.  we are populating the component manager hash with
factories that do not have constructors.  New patch coming up.

Comment 26

[Doug Turner (:dougt)]

Attachment: patch v.3

what jband said.

Comment 27

[John Bandhauer]

Comment on attachment 72012 [details] [diff] [review]
patch v.3

sr=jband.
I'm good with this. As I discussed with you...
- At minimum change 'mFactories' to some name that indicates that 
these are just the not-to-be-registered factories.
- The typedef of the struct to give it two typenames is not helpful.
- *I* would have given the struct a ctor and dtor that did something.

Comment 28

[Doug Turner (:dougt)]

Attachment: patch v.4

what jband said.

Comment 29

[Doug Turner (:dougt)]

Comment on attachment 72269 [details] [diff] [review]
patch v.4

carrying forward jband's sr=

Comment 30

[Alec Flett]

Comment on attachment 72269 [details] [diff] [review]
patch v.4

sr=alecf

Comment 31

[Asa Dotzler [:asa]]

Comment on attachment 72269 [details] [diff] [review]
patch v.4

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 32

[Doug Turner (:dougt)]

Checking in nsGenericFactory.cpp;
/cvsroot/mozilla/xpcom/components/nsGenericFactory.cpp,v  <--  nsGenericFactory.cpp
new revision: 1.40; previous revision: 1.39
done
Checking in nsGenericFactory.h;
/cvsroot/mozilla/xpcom/components/nsGenericFactory.h,v  <--  nsGenericFactory.h
new revision: 1.12; previous revision: 1.11
done


Now these two files can be moved into xpcom/glue.

Comment 33

[Doug Turner (:dougt)]

Attachment: Removes unneeded #include's and DEFINE_CID

RDF at one point needed to #include this stuff.  This patch cleans up their
cruft.	reviews please.

Comment 34

[Doug Turner (:dougt)]

this will not land for 99

Comment 35

[Doug Turner (:dougt)]

moving that last patch to another bug since it is less critical than the keyword
decoration that this bug has.

Marking fixed.  120474 will move this code into the glue lib.

Comment 36

[Doug Turner (:dougt)]

moving that last patch to another bug since it is less critical than the keyword
decoration that this bug has.

Marking fixed.  120474 will move this code into the glue lib.