112262 - move nsMemory to glue library

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Doug Turner (:dougt)]

Comment 1

[Doug Turner (:dougt)]

Attachment: Moves nsMemory to xpcom/glue

still need to figure out why I needed to manually #define the memory contract
id.

Comment 2

[Doug Turner (:dougt)]

Attachment: moves nsMemory to glue, second try with exit_routine()

Comment 3

[Doug Turner (:dougt)]

Attachment: same as above but includes new files.

Comment 4

[Doug Turner (:dougt)]

Attachment: same as above but short circuits free(null)

Comment 5

[Doug Turner (:dougt)]

Attachment: make it work on linux...

Comment 6

[Doug Turner (:dougt)]

Attachment: Add Unregister api

Comment 7

[rpotts (gone)]

Comment on attachment 59744 [details] [diff] [review]
Add Unregister api

sr=[e-mail]

Comment 8

[John Bandhauer]

Making a function call to EnsureGlobalMemory() on each call is lame. At one
point this static class had an inlined ensure check. How about...

#define ENSURE_ALLOCATOR \
  (gHasMemoryShutdown ? PR_FALSE : gMemory || SetupGlobalMemory())

static nsIMemory*
SetupGlobalMemory()
{
    NS_ASSERTION(!gMemory, "bad call");
    NS_GetMemoryManager(&gMemory);
    NS_ASSERTION(gMemory, "can't get memory manager!");
    NS_RegisterXPCOMExitRoutine(FreeGlobalMemory);
    return gMemory;
}

NS_EXPORT void*
nsMemory::Realloc(void* ptr, PRSize size)
{
    if (!ENSURE_ALLOCATOR)
        return nsnull;

    return gMemory->Realloc(ptr, size);
}

etc.


Now for more substantial issues...

Why are we putting the impl class in the glue library? Don't we risk there being
multiple versions of the allocator instantiated? Another option is to have these
glue versions just forward to the one and only shared impl. This would not be
hard to code. It is how I originally implemented this. It has more overhead, but
makes this static class what is supposed to be: just a convienence. I'm curious
if the overhead would be lost in the noise or not.

And, why jump through hoops to try to not leak the object? So what if we leak
this one object? It's the allocator, it has reason to outlive everything else. I
don't see that an exit list helps that much. What protects us from there being
some other object that also ends up in the exit list but which needs to use the
allocator? There is no promise about ordering here. I understand why we prohibit
creating xpcom instances while xpcom is shutting down, but to have there be any
doubt at all if the allocator is going to be dead when you need it is just wrong.

Comment 9

[Doug Turner (:dougt)]

>Making a function call to EnsureGlobalMemory() on each call is lame. At one
point this static class had an inlined ensure check. How about...

Sounds fine.

>Why are we putting the impl class in the glue library? Don't we risk there being
>multiple versions of the allocator instantiated? Another option is to have these
>glue versions just forward to the one and only shared impl. This would not be
>hard to code. It is how I originally implemented this. It has more overhead, but
>makes this static class what is supposed to be: just a convienence. I'm curious
>if the overhead would be lost in the noise or not.

Hmm... maybe I am missing something, this is exactly what the code does.  There
is only one nsIMemory implementation for all clients of nsMemory.  This class
just forwards the call as you described.  

>And, why jump through hoops to try to not leak the object? So what if we leak
>this one object? It's the allocator, it has reason to outlive everything else. I
>don't see that an exit list helps that much. 

Outliving other objects does not mean leaking the object.  If we ever hope to
support init-shutdown-init within a single application lifetime, we have to
clean up our mess.

>What protects us from there being some other object that also ends up in the
>exit list but which needs to use the allocator? There is no promise about
>ordering here. I understand why we prohibit creating xpcom instances while
xpcom >is shutting down, but to have there be any doubt at all if the allocator
is >going to be dead when you need it is just wrong.

Your right.  We have this same problem today.  And we leak the nsMemoryImpl
because of it.  Maybe this ExitRoutine API should be private and undocumented,
between only the glue library and xpcom proper.  In this way, we could have a
tighter control as to what is being added.  thoughts??

Comment 10

[John Bandhauer]

> Hmm... maybe I am missing something
No, I was being particularly dense. I misread what was where.

I'm thinking that perhaps releasing the allocator ought to be done at the end of
the module shutdown of whatever module the nsMemory shim lives in - rather than
when xpcom shuts down. But, heck that might be *earlier* than the end of xpcom
shutdown. Still, this would fit in with the concept of module unloading -
release your references to other modules as your module is taken down. I dunno,
maybe that has to be put off until later and what you have here is good enough.

Oh, and I realised that the code I suggested above is perhaps faster in the
normal case as:

#define ENSURE_ALLOCATOR \
  (gMemory ? PR_TRUE : !gHasMemoryShutdown && SetupGlobalMemory())

Of course, we're ignoring threadsafety issues completely here. One of the big
upsides of just leaking the reference to the allocator was that we knew that
once acquired we could not race with another thread that might be removing our
reference to it. Now...?

Comment 11

[Doug Turner (:dougt)]

Attachment: use macro & makes exit routine private.

Comment 12

[John Bandhauer]

Comment on attachment 60445 [details] [diff] [review]
use macro & makes exit routine private.

> nsMemoryImpl::Free(void * ptr)
> {
>+    if (!ptr) return;
>     PR_Free(ptr);
> }

Where did this come from? Do you really want to change this contract?
This pays off only if everyone knows about it. Otherwise, you just
pay the cost of double null checks on tons of code paths.

>+ * Private Method to register an exit routine.  This method
>+ * allows you to setup a callback that will be called from 
>+ * the NS_ShutdownXPCOM function after all services and 
>+ * components have gone away.
>+ *
>+ * This API is for the exclusive use of the xpcom glue library.

Does this need to be private? I thought out discussion ended with
the idea that by adding a priority concept (and using a 'last-ish'
priority for the allocator shutdown) that we could makde this public.
People will either just use it anyway or we'll end up inventing a
parallel system anyway, no?

Instead you're punting on implmenting the priorities marking it private 
and hoping no one will try to use it (or assume that the priorities
system works. I'm not totally against this, it's just iffy.

>-#ifdef PAGE_MANAGER
>-#include "nsPageMgr.h"
>-#endif

Whatever that was, I guess we don't need it :)

>+// gMemory will be freed during shutdown. 
>+static nsIMemory* gMemory = nsnull;
>+nsresult NS_COM NS_GetMemoryManager(nsIMemory* *result)
>+{
>+    nsresult rv = NS_OK;
>+    if (!gMemory)
>+    {
>+        nsCOMPtr<nsIMemory> memory;
>+        rv = nsMemoryImpl::Create(nsnull,
>+                                  NS_GET_IID(nsIMemory), 
>+                                  (void**)&gMemory);
>+    }
>+    NS_IF_ADDREF(*result = gMemory);
>+    return rv;
>+}

You don't use that nsCOMPtr memory.

>+#define ENSURE_ALLOCATOR \
>+  (gMemory ? PR_TRUE : (gHasMemoryShutdown ? PR_TRUE : SetupGlobalMemory()))
>+
>+
...
>+NS_EXPORT void*
>+nsMemory::Alloc(PRSize size)
>+{
>+    ENSURE_ALLOCATOR;
>+    if (!gMemory)
>+        return nsnull;
>+
>+    return gMemory->Alloc(size);
>+}

You missed my point for the macro. Your's tests gMemory twice for
no good reason. Please look again at what I suggested...

#define ENSURE_ALLOCATOR \
  (gMemory ? PR_TRUE : !gHasMemoryShutdown && SetupGlobalMemory())

static nsIMemory*
SetupGlobalMemory()
{
    NS_ASSERTION(!gMemory, "bad call");
    NS_GetMemoryManager(&gMemory);
    NS_ASSERTION(gMemory, "can't get memory manager!");
    NS_RegisterXPCOMExitRoutine(FreeGlobalMemory);
    return gMemory;
}

NS_EXPORT void*
nsMemory::Realloc(void* ptr, PRSize size)
{
    if (!ENSURE_ALLOCATOR)
	return nsnull;

    return gMemory->Realloc(ptr, size);
}

Is there a good reason to not do it this way?

Comment 13

[Doug Turner (:dougt)]

the short circuiting free(null) was rpotts' idea.  He was seeing 1% of all calls
to nsMemory::Free() called with null.  I think that you are right though.  May
people are checking for null prior to calling ::Free().  I have removed it from
my changes.

As for the exit routine stuff, I do not like exposing this outside of xpcom.  We
already have a notification which is sent to anyone interested in xpcom
shutdown.  After that point, most of XPCOM is useless for the client code. 
Futhermore, the embedding application could hook some a similar system when
their call to shutdown xpcom exits.  I do not buy that a xpcom client really
needs this.  maybe this will bite me in the future, but for now, I think that
this is a "private frozen" API.

I will wrap up the other changes and repost a new patch.  John, thanks.

Comment 14

[John Bandhauer]

> the short circuiting free(null) was rpotts' idea...

I'm not against this if we follow through and get the word out and get people
removing unnecessary null checks. Most people know that 'delete' is null-safe.
But I expect that most programmers don't expect 'free' to be null-safe. We can
always do this later.

> As for the exit routine stuff...

I buy that. Thanks.

Comment 15

[Doug Turner (:dougt)]

Attachment: uses marcro's correctly. removes free(null) shortcircuit.

Comment 16

[John Bandhauer]

Comment on attachment 60665 [details] [diff] [review]
uses marcro's correctly. removes free(null) shortcircuit.

+    if (!ptr) return;

Well, actually you didn't remove the null check.

+NS_EXPORT nsresult
+nsMemory::HeapMinimize(PRBool aImmediate)
+{
+    if (!ENSURE_ALLOCATOR)
+	 return nsnull;    
+
+    return gMemory->HeapMinimize(aImmediate);
+}

You need to return an error code not null (which looks a lot like NS_OK)

+NS_EXPORT nsIMemory*
+nsMemory::GetGlobalMemoryService()
+{
+    if (!gMemory)
+	 nsresult rv = NS_GetMemoryManager(&gMemory);
+   
+    nsIMemory* result = gMemory;
+    NS_IF_ADDREF(result);
+    return result;
+}

Can't this bypass the shutdown stuff. You don't want that to happen do you?

Shouldn't this be?...

NS_EXPORT nsIMemory*
nsMemory::GetGlobalMemoryService()
{
    if (!ENSURE_ALLOCATOR)
	return nsnull;

    NS_ADDREF(result = gMemory;);
    return result;
}

fix those issues and r/sr=jband

Comment 17

[Doug Turner (:dougt)]

Checking in base/MANIFEST;
/cvsroot/mozilla/xpcom/base/MANIFEST,v  <--  MANIFEST
new revision: 3.15; previous revision: 3.14
done
Checking in base/Makefile.in;
/cvsroot/mozilla/xpcom/base/Makefile.in,v  <--  Makefile.in
new revision: 1.44; previous revision: 1.43
done
Checking in base/makefile.win;
/cvsroot/mozilla/xpcom/base/makefile.win,v  <--  makefile.win
new revision: 1.41; previous revision: 1.40
done
Checking in base/nsMemoryImpl.cpp;
/cvsroot/mozilla/xpcom/base/nsMemoryImpl.cpp,v  <--  nsMemoryImpl.cpp
new revision: 1.18; previous revision: 1.17
done
Checking in base/nsMemoryImpl.h;
/cvsroot/mozilla/xpcom/base/nsMemoryImpl.h,v  <--  nsMemoryImpl.h
new revision: 1.6; previous revision: 1.5
done
Checking in build/makefile.win;
/cvsroot/mozilla/xpcom/build/makefile.win,v  <--  makefile.win
new revision: 1.32; previous revision: 1.31
done
Checking in build/nsXPCOM.h;
/cvsroot/mozilla/xpcom/build/nsXPCOM.h,v  <--  nsXPCOM.h
new revision: 1.2; previous revision: 1.1
done
RCS file: /cvsroot/mozilla/xpcom/build/nsXPCOMPrivate.h,v
done
Checking in build/nsXPCOMPrivate.h;
/cvsroot/mozilla/xpcom/build/nsXPCOMPrivate.h,v  <--  nsXPCOMPrivate.h
initial revision: 1.1
done
Checking in build/nsXPComInit.cpp;
/cvsroot/mozilla/xpcom/build/nsXPComInit.cpp,v  <--  nsXPComInit.cpp
new revision: 1.118; previous revision: 1.117
done
Checking in glue/MANIFEST;
/cvsroot/mozilla/xpcom/glue/MANIFEST,v  <--  MANIFEST
new revision: 1.2; previous revision: 1.1
done
Checking in glue/Makefile.in;
/cvsroot/mozilla/xpcom/glue/Makefile.in,v  <--  Makefile.in
new revision: 1.4; previous revision: 1.3
done
Checking in glue/makefile.win;
/cvsroot/mozilla/xpcom/glue/makefile.win,v  <--  makefile.win
new revision: 1.4; previous revision: 1.3
done
RCS file: /cvsroot/mozilla/xpcom/glue/nsMemory.cpp,v
done
Checking in glue/nsMemory.cpp;
/cvsroot/mozilla/xpcom/glue/nsMemory.cpp,v  <--  nsMemory.cpp
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/xpcom/glue/nsMemory.h,v
done
Checking in glue/nsMemory.h;
/cvsroot/mozilla/xpcom/glue/nsMemory.h,v  <--  nsMemory.h
initial revision: 1.1
done

checked in.  thanks rick and john.  

Comment 18

[Doug Turner (:dougt)]

changes successfully landed.