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.

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

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?

> 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.

> 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 :)

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 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 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 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

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 on attachment 165472 [details] [diff] [review] Export memory-management functions, rev. 3.1 /me grins evilly in shaver's direction

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.)

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 :-)

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

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

Confirming MinGW build now works fine. Thanks for that.

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.

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?

(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.

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

(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?

no, that's bug 271313