267767 - Make XPCOM memory management functions frozen exports

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Benjamin Smedberg]

We currently waste cycles with XPCOM memory allocation by making all allocations
go through a nsIMemory vtable. This is pointless, and I've made three XP_alloc
XP_realloc XP_free exports. Then the nsMemory static functions have inline
implementations which forward to these.

Comment 1

[Benjamin Smedberg]

Attachment: Export memory-management functions

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

hm... why name this XP_alloc? isn't that inconsistent with our usual NS_ prefix?
why not NS_Malloc, NS_Realloc, NS_Free?

Comment 3

[Darin Fisher]

Comment on attachment 164611 [details] [diff] [review]
Export memory-management functions

>Index: xpcom/base/nsIMemory.idl

> interface nsIMemory : nsISupports
...
>+     * @status DEPRECATED (use XP_alloc instead)
>+     *
...
>+     * @status DEPRECATED (use XP_realloc instead)
>+     *
...
>+     * @status DEPRECATED (use XP_free instead)
>+     *

I disagree with marking methods on this interface as deprecated.  Marking
something deprecated means that it is going away in the future.  We
certainly are not going to eliminate methods on this vtable, nor are we
going to eliminate the interface.

The point of this patch should be to move people away from nsMemory::
methods.  If anything is to be marked deprecated, it would seem that
those methods should be the ones marked as such.

NOTE: nsIMemory is used for a lot of good purposes internally within
Mozilla.  Case in point, see nsIPipe.  It allows necko to manaage a cache
of buffers that the pipe uses.


>Index: xpcom/base/nsMemoryImpl.cpp
>Index: xpcom/base/nsMemoryImpl.h

I have not reviewed these changes.


>Index: xpcom/build/Makefile.in

>-		-DEXPORT_XPTI_API
>+		-DEXPORT_XPTI_API \
>+		-D_IMPL_NS_STRINGAPI \

why are you adding this define here?  nsStringAPI.cpp is currently
built into libxpcom.so, so there should be no need to add this
define here.  what am i missing?


>Index: xpcom/build/nsXPCOM.h

>+# define XP_alloc                    XP_alloc_P
>+# define XP_realloc                  XP_realloc_P
>+# define XP_free                     XP_free_P

ACK... please use NS_ prefix!  Let's try to keep some sort of
consistency with our naming conventions.  XPCOM doesn't need a
new naming convention.

I think these should be named:

NS_Alloc
NS_Realloc
NS_Free


>+ * If ptr is null, this function behaves like malloc.

And, If s is 0, this function behaves like NS_Free.


>Index: xpcom/build/nsXPCOMPrivate.h

>+#include "nsStringAPI.h"

>-typedef nsresult   (* CStringToUTF16)(const nsACString &, PRUint32, const nsAString &);
>-typedef nsresult   (* UTF16ToCString)(const nsAString &, PRUint32, const nsACString &);
>+typedef nsresult   (* CStringToUTF16)(const nsACString &, nsCStringEncoding, nsAString &);
>+typedef nsresult   (* UTF16ToCString)(const nsAString &, nsCStringEncoding, nsACString &);

ack!  good catch... but maybe don't include nsStringAPI.h because
that will conflict with anyone that includes nsAString.h.  those
|PRUint32|'s should be |PRIntn| instead.  maybe use PRIntn instead
of nsCStringEncoding?  C/C++ says that sizeof(enum) == sizeof(int)

in fact, this bug fix should probably be ported to the mozilla 1.7
branch for the next moz 1.7 stable release.  please file a bug for
that! :)


>Index: xpcom/glue/Makefile.in

>+REQUIRES	= string

because of nsXPCOMPrivate.h including nsStringAPI.h rightright?

Comment 4

[Benjamin Smedberg]

> I disagree with marking methods on this interface as deprecated.  Marking

Is there another marking that says "use this other method instead"?

I had to use _IMPL_NS_STRINGAPI so that I could include nsStringAPI.h in
nsXPComInit.cpp. If you set _IMPL_NS_STRINGAPI then nsAString.h doesn't conflict
with nsStringAPI.h.

I had to include nsStringAPI.h in nsXPComInit.cpp so that I could take the
address of the string API functions in NS_GetFrozenFunctions, instead of
dynamically loading the functions by name (which is silly).

My copy (not the latest version) says that in C++ sizeof(enum) is
compiler-defined, but not larger than sizeof(int) unless the enumeration
contains a value larger than can be contained by "int", but I don't see anything
stating that it cannot be smaller than sizeof(int). Which is why I didn't want
to freeze this as an enum in the first place.

Comment 5

[Darin Fisher]

> Is there another marking that says "use this other method instead"?

How about a comment in nsIMemory.idl above the interface declaration that
directs people to the new memory allocation methods.  The idea is to encourage
use of the methods.


> I had to include nsStringAPI.h in nsXPComInit.cpp so that I could take the
> address of the string API functions in NS_GetFrozenFunctions, instead of
> dynamically loading the functions by name (which is silly).

Ah, of course!  That makes sense.  Perhaps we should make nsStringAPI.h look at
MOZILLA_STRICT_API instead, since if that define is set it is clear that
nsAString.h must not be included.  That way we don't have to muck with
_IMPL_NS_STRINGAPI, which is really meant only to be defined when compiling
nsStringAPI.cpp :-)


> My copy (not the latest version) says that in C++ sizeof(enum) is
> compiler-defined, but not larger than sizeof(int) unless the enumeration
> contains a value larger than can be contained by "int", but I don't see anything
> stating that it cannot be smaller than sizeof(int). Which is why I didn't want
> to freeze this as an enum in the first place.

OK, this is very interesting.  OK, I'm fine with your patch, provided we can
find some way to use MOZILLA_STRICT_API as the switch instead of
_IMPL_NS_STRINGAPI :)

Comment 6

[timeless]

NS_Realloc and NS_Free should clearly indicate that ptr must be the result of
NS_(Rea|A)lloc if it is non 0

-REQUIRES= $(NULL)
+REQUIRES= string
be nice and add \ (\n) $(NULL)

I'm not sure about the change overall, i'll need to think about it.

You should probably declare whether these functions are
threadsafe/freethreaded/mainthreadonly/...

Comment 7

[Benjamin Smedberg]

Attachment: Export memory-management functions, rev. 2

Comment 8

[Darin Fisher]

Comment on attachment 165093 [details] [diff] [review]
Export memory-management functions, rev. 2

>Index: xpcom/base/nsMemoryImpl.cpp

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

use NS_Free for consistency, or at least add a comment that warns
about keeping this function in sync with NS_Free.


>+nsresult 
>+nsMemoryImpl::Startup()
>+{
>+    sFlushLock = PR_NewLock();
>+    if (!sFlushLock) return NS_ERROR_FAILURE;
>+
>+#ifdef NS_MEMORY_FLUSHER_THREAD
>+    return kGlobalMemoryFlusher.Init();
>+#else
>+    return NS_OK;
>+#endif
>+}

is it possible that we only need to allocate sFlushLock when
NS_MEMORY_FLUSHER_THREAD is defined?


>+    kGlobalMemory.RunFlushers(event->mReason);

use of the 'k' prefix like this feels odd.  it's an object, so
how about using the 's' prefix instead?


>+    NS_IMETHOD_(nsrefcnt) AddRef(void) { return NS_OK; }
>+    NS_IMETHOD_(nsrefcnt) Release(void) { return NS_OK; }

NS_OK doesn't quite make sense here... how about "1" instead?


>+static MemoryFlusher kGlobalMemoryFlusher;

sGlobalMemoryFlusher ;-)



>+    NS_IMETHOD_(nsrefcnt) AddRef(void) { return NS_OK; }
>+    NS_IMETHOD_(nsrefcnt) Release(void) { return NS_OK; }

s/NS_OK/1/


>Index: xpcom/build/nsXPCOM.h


>+ * If ptr is null, this function behaves like malloc.

s/malloc/NS_Alloc/


>+ * @param ptr   The block of memory to free. This block must originally have
>+ *              been allocated by NS_Alloc or NS_Realloc

It's important to state that NS_Free(0) is allowed.

You should probably also state that the allocator is threadsafe as
timeless queried just so there is no confusion on that point.


>Index: xpcom/build/nsXPComInit.cpp

> #define GET_FUNC(_tag, _decl, _name)                        \
>-  functions->_tag = (_decl) PR_FindSymbol(xpcomLib, _name); \
>-  if (!functions->_tag) goto end
>+  functions->_tag = &_name;
> 
> nsresult NS_COM PR_CALLBACK
> NS_GetFrozenFunctions(XPCOMFunctions *functions, const char* libraryPath)

I think this means that you need to move NS_GetFrozenFunctions into
xpcom/stub/nsXPComStub.cpp otherwise this won't link on some systems.


Looks like diffs for nsStringAPI.h didn't make it into the patch.

Comment 9

[Benjamin Smedberg]

Attachment: Export memory-management functions, rev. 3

Comment 10

[Benjamin Smedberg]

Comment on attachment 165299 [details] [diff] [review]
Export memory-management functions, rev. 3

I forgot to add the nits about threadsafe and NS_Alloc; they will be added
before checkin. Let me know if you want another patch.

Comment 11

[Darin Fisher]

Comment on attachment 165299 [details] [diff] [review]
Export memory-management functions, rev. 3

>+nsMemoryImpl::Startup()
>+{
>+    sFlushLock = PR_NewLock();
>+    if (!sFlushLock) return NS_ERROR_FAILURE;
>+
>+#ifdef NS_MEMORY_FLUSHER_THREAD
>+    return sGlobalMemoryFlusher.Init();
>+#else
>+    return NS_OK;
>+#endif
>+}

again, i'm wondering if we can do away with this lock when
NS_MEMORY_FLUSHER_THREAD is not defined.  any thoughts?


>+static const XPCOMFunctions kFrozenFunctions = {

/me likes :)


>+extern "C" NS_EXPORT nsresult
>+NS_GetFrozenFunctions(XPCOMFunctions *functions, const char* /* libraryPath */)
>+{
>+    if (!functions)
>+        return NS_ERROR_OUT_OF_MEMORY;
>+
>+    if (functions->version != XPCOM_GLUE_VERSION)
>+        return NS_ERROR_FAILURE;
>+
>+    PRUint32 size = functions->size;
>+    if (size > sizeof(XPCOMFunctions))
>+        size = sizeof(XPCOMFunctions);

hmm... in fact, perhaps we should null out the rest of the symbols
or perhaps we should return an error in this case.  afterall, we
do not promise backwards compatibility, only forwards compatibility.
so, if someone compiles against mozilla 1.8, we don't want their
component to be used in mozilla 1.7.  perhaps this should return an
error instead?	or maybe nsXPCOMGlue.cpp takes care of this for us
by null checking each function pointer.  hmm...


r=darin

Comment 12

[Benjamin Smedberg]

Attachment: Export memory-management functions, rev. 3.1

I put a little bit more in the #ifdef NS_MEMORY_FLUSHER_THREAD, fixed the
comments, and fixed the memory flusher shutdown (I needed to scope the
nsAutoLock in StopAndJoin a little more carefully, so that I wasn't holding the
lock while joining the thread). I don't think we need to worry about
XPCOMFunctions being too large: the glue nulls out the memory in advance, and I
don't think we want to exclude the possibility that somebody might want to use
a later glue with an earlier XPCOM if they know exactly what they're doing.

Comment 13

[Benjamin Smedberg]

Comment on attachment 165472 [details] [diff] [review]
Export memory-management functions, rev. 3.1

/me grins evilly in shaver's direction

Comment 14

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

Comment on attachment 165472 [details] [diff] [review]
Export memory-management functions, rev. 3.1

Looks good to me, but I'd like to make NS_Alloc safe to call with a zero size,
like malloc(3) is defined to be.  Just bump it to 1 if a zero comes in.

(And I now wonder if we still need to call PR_Malloc, now that we don't have to
care about OS 9.)

Comment 15

[Darin Fisher]

one reason to stick with PR_Malloc is that PR_Malloc is not necessarily
equivalent to malloc since environment variables can be used to enable the NSPR
zone allocator, whatever that is.  and, since nsMemory::Alloc has always equaled
PR_Malloc, we might get into trouble if we change it now.  that said, i'd almost
be willing to give it a try :-)

Comment 16

[David G King]

The checkin for this bug is causing the following on Win32/MinGW/cygwin :-

e:/mozilla/source/mozilla/xpcom/base/nsMemoryImpl.cpp:395: error: extra `;'
make[4]: *** [nsMemoryImpl.o] Error 1
make[4]: Leaving directory `/cygdrive/e/mozilla/source/mozilla/xpcom/base'
make[3]: *** [libs] Error 2
make[3]: Leaving directory `/cygdrive/e/mozilla/source/mozilla/xpcom'
make[2]: *** [libs] Error 2
make[2]: Leaving directory `/cygdrive/e/mozilla/source/mozilla'
make[1]: *** [alldep] Error 2
make[1]: Leaving directory `/cygdrive/e/mozilla/source/mozilla'
make: *** [alldep] Error 2

Comment 17

[Benjamin Smedberg]

Fixed, along with extra-semicolon-ectomy for mingw/gcc3.4

Comment 18

[David G King]

Confirming MinGW build now works fine. Thanks for that.

Comment 19

[Benjamin Smedberg]

For the record, this patch did not have nearly the affect on Ts or Txul that I
expected. We got a small codesize win (4k for dynamic builds, 8k for static mac
build, no statistics for windows?). Now we should consider ditching PR_Malloc
and using stdlib malloc directly. I'll file followup bugs.

Comment 20

[Alfred Kayser]

Question: It seems that sFlushLock only gets initialized when
NS_MEMORY_FLUSHER_THREAD is set:
233 #ifdef NS_MEMORY_FLUSHER_THREAD
234     sFlushLock = PR_NewLock();
235     if (!sFlushLock) return NS_ERROR_FAILURE;

But sFlushLock is used in FlushMemory and RunFlushers:
290         // Are we already flushing?
291         nsAutoLock l(sFlushLock);

and destroyed in ::Shutdown:
250     if (sFlushLock) PR_DestroyLock(sFlushLock);
251     sFlushLock = nsnull;

Is sFlushLock truely only used in NS_MEMORY_FLUSHER_THREAD mode, or not?

Comment 21

[Worcester12345]

(In reply to comment #3)

> >+ * If ptr is null, this function behaves like malloc.
> 
> And, If s is 0, this function behaves like NS_Free.
> 
> 
> >Index: xpcom/build/nsXPCOMPrivate.h
> 
> >+#include "nsStringAPI.h"
> 
> >-typedef nsresult   (* CStringToUTF16)(const nsACString &, PRUint32, const
nsAString &);
> >-typedef nsresult   (* UTF16ToCString)(const nsAString &, PRUint32, const
nsACString &);
> >+typedef nsresult   (* CStringToUTF16)(const nsACString &, nsCStringEncoding,
nsAString &);
> >+typedef nsresult   (* UTF16ToCString)(const nsAString &, nsCStringEncoding,
nsACString &);
> 
> ack!  good catch... but maybe don't include nsStringAPI.h because
> that will conflict with anyone that includes nsAString.h.  those
> |PRUint32|'s should be |PRIntn| instead.  maybe use PRIntn instead
> of nsCStringEncoding?  C/C++ says that sizeof(enum) == sizeof(int)
> 
> in fact, this bug fix should probably be ported to the mozilla 1.7
> branch for the next moz 1.7 stable release.  please file a bug for
> that! :)


Did this additional bug ever get filed? Have a bug #?

Just a comment: I am very impressed reading through this bug with the
interaction of developers towards solving a problem. 

Comment 22

[Doug Turner (:dougt)]

This patch causes crashes if you call nsMemory::HeapMinimize(PR_TRUE)

It looks like the sFlushLock is not initalized before being used
It also looks like this lock needs to either be inside or outside of the
NS_MEMORY_FLUSHER_THREAD

Comment 23

[Worcester12345]

(In reply to comment #22)
> This patch causes crashes if you call nsMemory::HeapMinimize(PR_TRUE)
> 
> It looks like the sFlushLock is not initalized before being used
> It also looks like this lock needs to either be inside or outside of the
> NS_MEMORY_FLUSHER_THREAD

Should it be reopened?

Comment 24

[timeless]

no, that's bug 271313