Last Comment Bug 112262 - move nsMemory to glue library
: move nsMemory to glue library
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: x86 Windows 2000
: -- normal (vote)
: mozilla0.9.7
Assigned To: Doug Turner (:dougt)
: Scott Collins
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2001-11-27 14:57 PST by Doug Turner (:dougt)
Modified: 2001-12-07 23:49 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Moves nsMemory to xpcom/glue (15.96 KB, patch)
2001-11-27 14:59 PST, Doug Turner (:dougt)
no flags Details | Diff | Review
moves nsMemory to glue, second try with exit_routine() (10.25 KB, patch)
2001-11-27 16:49 PST, Doug Turner (:dougt)
no flags Details | Diff | Review
same as above but includes new files. (17.59 KB, patch)
2001-11-27 16:53 PST, Doug Turner (:dougt)
no flags Details | Diff | Review
same as above but short circuits free(null) (17.77 KB, patch)
2001-11-29 10:58 PST, Doug Turner (:dougt)
no flags Details | Diff | Review
make it work on linux... (18.26 KB, patch)
2001-11-29 13:44 PST, Doug Turner (:dougt)
no flags Details | Diff | Review
Add Unregister api (18.60 KB, patch)
2001-11-29 14:07 PST, Doug Turner (:dougt)
rpotts: superreview+
Details | Diff | Review
use macro & makes exit routine private. (22.25 KB, patch)
2001-12-04 19:17 PST, Doug Turner (:dougt)
no flags Details | Diff | Review
uses marcro's correctly. removes free(null) shortcircuit. (22.56 KB, patch)
2001-12-06 10:46 PST, Doug Turner (:dougt)
jband_mozilla: superreview+
Details | Diff | Review

Description Doug Turner (:dougt) 2001-11-27 14:57:46 PST
 
Comment 1 Doug Turner (:dougt) 2001-11-27 14:59:44 PST
Created attachment 59401 [details] [diff] [review]
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) 2001-11-27 16:49:02 PST
Created attachment 59440 [details] [diff] [review]
moves nsMemory to glue, second try with exit_routine()
Comment 3 Doug Turner (:dougt) 2001-11-27 16:53:28 PST
Created attachment 59442 [details] [diff] [review]
same as above but includes new files.
Comment 4 Doug Turner (:dougt) 2001-11-29 10:58:03 PST
Created attachment 59718 [details] [diff] [review]
same as above but short circuits free(null)
Comment 5 Doug Turner (:dougt) 2001-11-29 13:44:03 PST
Created attachment 59739 [details] [diff] [review]
make it work on linux...
Comment 6 Doug Turner (:dougt) 2001-11-29 14:07:15 PST
Created attachment 59744 [details] [diff] [review]
Add Unregister api
Comment 7 rpotts (gone) 2001-11-29 14:08:52 PST
Comment on attachment 59744 [details] [diff] [review]
Add Unregister api

sr=rpotts@netscape.com
Comment 8 John Bandhauer 2001-11-29 21:50:49 PST
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) 2001-11-29 22:35:13 PST
>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 2001-11-30 06:43:14 PST
> 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) 2001-12-04 19:17:50 PST
Created attachment 60445 [details] [diff] [review]
use macro & makes exit routine private.
Comment 12 John Bandhauer 2001-12-05 12:23:52 PST
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) 2001-12-05 12:44:18 PST
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 2001-12-05 12:50:45 PST
> 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) 2001-12-06 10:46:48 PST
Created attachment 60665 [details] [diff] [review]
uses marcro's correctly. removes free(null) shortcircuit.
Comment 16 John Bandhauer 2001-12-06 11:02:35 PST
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) 2001-12-07 12:28:28 PST
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) 2001-12-07 23:49:07 PST
changes successfully landed.

Note You need to log in before you can comment on or make changes to this bug.