nsFactoryEntry maintains type string and loader

RESOLVED FIXED in mozilla0.9.4

Status

()

defect
P3
normal
RESOLVED FIXED
18 years ago
18 years ago

People

(Reporter: dp, Assigned: dp)

Tracking

Trunk
mozilla0.9.4
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

nsFactoryEntry::
   nsCString type;
   nsIComponentLoader *loader;


We create about 700 of these nsFactoryEntries. nsCString is about 16 bytes +
each type is about 32 bytes

Maintaining this costs:

- 700 allocs
- 700 copies
- 700 * (16 + 32 + 4) = 36k memory usage

+ the loaders are maintained in a hash table - hashed on the type string. We
have about 2 loaders.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.5
Priority: -- → P3
*** Bug 96464 has been marked as a duplicate of this bug. ***
r=valeski
Blocks: 7251
+#define NS_MAX_COMPONENT_TYPES 6
...
+    struct nsComponentData mCData[NS_MAX_COMPONENT_TYPES];

This does not make me happy. Why not an nsVoidArray and let the world add as
many loaders as they desire?

And isn't this 'loader data' rather than 'component data'?
dp was/is trying to minimize things. I was sold on the raw array :-). I too
asked about the size thing. His point was that we really only ever have a couple
few entries, and that list isn't likely to grow. makes sense to me, and we lose
the cost (albeit minimal) of the void array. if losing the dynamic sizing is an
issue, I'll easily side w/ the fact that this thing is only created once, and we
only ever have one of them in the system. 

going to a void array *might* throw off the indexing assumptions. dp, if you do
this, please verify that those are still valid.
Jband convinced me that using nsVoidArray is better. I will change the patch.
Ccing jband
+    typedef struct nsLoaderdata {
+        nsIComponentLoader *loader;
+        const char *type;
+    } nsLoaderdata;
+
+    struct nsLoaderdata *mLoaderData;

This is C++. Why the typedef and why 'struct' for the member? Why not just?...

    struct nsLoaderdata {
        nsIComponentLoader *loader;
        const char *type;
    };

    nsLoaderdata *mLoaderData;


+nsComponentManagerImpl::AddLoaderType(const char *typeStr)
+{
+    int typeIndex = GetLoaderType(typeStr);
+    if (typeIndex > 0)
+        return typeIndex;
You should *at least* assert that typeIndex != 0. If someone tries to add the
native type you'd want to hear about it.

+    if (mNLoaderData >= mMaxNLoaderData) {

mNLoaderData > mMaxNLoaderData would be an error case you'd like to assert
about, no? You don't want to silently allow it and have something odd happen

You need to add a line to AddLoaderType to set mLoaderData[typeIndex].loader to
nsnull.

I don't see any reason to expose NS_COMPONENT_TYPE_NATIVE in the public header.
and I think the same for NS_COMPONENT_TYPE_STATIC (and more so - since it might
not match up built w/o the static loader). What is the thinking here? Who would
care about these magic numbers? Why not hide them?

Are all calls into this new code serialized (multithread-wise)? Or is that just
a different bug?

The other changes look OK. I think shaver or (now) dougt ought to be looking
close at changes here to catch possible errors that peop[le not so close to this
code might miss.
> This is C++. Why the typedef and why 'struct' for the member? Why not just?...

Sure. Accepted. Done.

> You should *at least* assert that typeIndex != 0. If someone tries to add the
> native type you'd want to hear about it.

The semantics of AddLoaderType() is get if available, add otherwise. It is
possible for code in the initializer to call this to add the native loader. I
just thought it was cleaner and more evident if I did that in Init() myself. So
I dont know if I would want to add the ASSERT. I will however change this to
if (typeIndex >= 0) instead of (typeIndex > 0)

> +    if (mNLoaderData >= mMaxNLoaderData) {
>
> mNLoaderData > mMaxNLoaderData would be an error case you'd like to assert
> about, no? You don't want to silently allow it and have something odd happen

Accepted. Added the assertion.

> You need to add a line to AddLoaderType to set mLoaderData[typeIndex].loader
> to nsnull.

Done

> I don't see any reason to expose NS_COMPONENT_TYPE_NATIVE in the public header.

I think you are confusing nsComponentManager.h and nsIComponentManager.h The
former is private and later is public.

> Are all calls into this new code serialized (multithread-wise)? Or is that
just a different bug?

I thought about it. The code path hasn't changed. Only the maintenance of data
has changed. So atleast I would say this wont add to the problem. I dont yet
know if there is a problem. Atleast we I know we dont know yet as we are not
adding any loaders at all. Only the native loader is in action.

> The other changes look OK. I think shaver or (now) dougt ought to be looking
> close at changes here to catch possible errors that peop[le not so close to
> this code might miss.

Will get them to review this.

Ccing Doug the new owner of xpcom.
+// Predefined loader types. Do not change the numbers.
+// NATIVE should be 0 as it is being used as the first array index.
+#define NS_COMPONENT_TYPE_NATIVE 0
+#define NS_COMPONENT_TYPE_STATIC (NS_COMPONENT_TYPE_NATIVE + 1)
+

> I think you are confusing nsComponentManager.h and nsIComponentManager.h The
> former is private and later is public.

That's fine. You are not *publically* exporting them. But you are exporting them
to the other files that #include nsComponentManager.h. You have not said why
they have any more reason to be used beyond nsComponentMaanger.cpp than
NS_LOADER_DATA_ALLOC_STEP. Is this not the same level of implementation detail?
Or are you going to let other callers make assumptions about those numbers? The
comment implies that these numbers are special and others might follow. But you
don't *use* NS_COMPONENT_TYPE_STATIC anywhere - so I don't know why it is
#defined. And unless callers really are going to be using
NS_COMPONENT_TYPE_NATIVE, then you could just as well use the literal 0
internally - you use the literal -1 and the internals *know* this is a zero
based array. 

I don't care if NS_COMPONENT_TYPE_NATIVE is #define'd or is in the header or
not. (And this issue is hardly worth spending any time on!) But, I don't see why
the other one, NS_COMPONENT_TYPE_STATIC, exists. And I'm wondering if you have
future plans to have callers use explicit #define'd index values.
Ah! ok. Here is my feeble thinking:

The native and static component loaders are provided by xpcom. Other loaders can
be written and added to a loader-category and xpcom will pick it up in its
enumeration process and service components of those types too.

nsComponentManagerImpl::GetLoaderForType(int type, ...) can be used to get a
loader of a certain type. NATIVE or STATIC can be passed into these. So I
thought the default nature of these loaders should exist along with the
definition of the API that is using it.

ALLOC_STEP could have very well gone in nsComponentManager.h but since the api
wasn't ever using it and it was pure implementation, I decided to put it in the
.cpp file.
STATIC is defined 1+NATIVE. nsComponentManagerImpl::Init() adds this loader type
into mLoaderData after adding NATIVE.

I will remove the TYPE_STATIC #define. It isn't being used. We can add it in
when the requirement occurs. I wont really mind moving the TYPE_NATIVE into the
.cpp file if you think that is better and this way is confusing.
sr=jband
I assume that you've like tested and stuff.
what jband said. r=me.
a=asa on behalf of drivers.
     if (NS_FAILED(rv))
-    if (rv == NS_ERROR_REG_NOT_FOUND)
-        /* missing componentType, we assume application/x-moz-native */
-        componentType = nativeComponentType;
+        if (rv == NS_ERROR_REG_NOT_FOUND) {
+            /* missing componentType, we assume application/x-moz-native */
+            type = NS_COMPONENT_TYPE_NATIVE;
+        }
     else
         return rv;              // XXX translate error code?

This looks like dangling else, but isn't (the else return rv; properly goes with
the inner if) -- please indent that else return rv; and maybe brace the whole
outer if's then part.  Might also read better to invert the inner if's condition
and return rv if rv != NS_ERROR_REG_NOT_FOUND.

/be
Fix checked into 0.9.4

Will do the indentation.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Target Milestone: mozilla0.9.5 → mozilla0.9.4
Blocks: 46832
You need to log in before you can comment on or make changes to this bug.