In bug 504640, we found that finalizers were the main cause of GC pauses. We propose running finalizers on a separate thread. js_GC would only collect the finalizers to be run and then hand them off to a new thread. js_GC runs finalizers in place only if memory cannot be allocated to store the pointers to be finalized.

Based on analysis by dmandelin, and with a bunch of suggestions by shaver. Completely untested, uploading for feedback only.

The patch seems to work great. The animation is much smoother now. top shows a max cpu utilization of 104% with a 3.5.1 product build. With TM tip and this patch applied, it spikes to 138%, indicating that the 2nd thread is busy freeing things on the 2nd core. I also get about 30% higher fps than with stock 3.5.1. That might be in part due to other VM improvements and some local GC/allocator path patches.

I get a malloc failure after a while running this. Not sure whether this happens without the patch too. We might want to bump the priority of the deallocation thread. Tue Jul 21 21:45:01 host-2-27.mv.mozilla.com firefox-bin[382] <Error>: CGBitmapContextCreateImage: failed to allocate 1800000 bytes. Tue Jul 21 21:45:01 host-2-27.mv.mozilla.com firefox-bin[382] <Error>: CGImageCreate: invalid image provider: NULL. firefox-bin(382,0xa07c7720) malloc: *** mmap(size=1802240) failed (error code=12) *** error: can't allocate region *** set a breakpoint in malloc_error_break to debug

Comment on attachment 389857 [details] [diff] [review] disable async free's in ST shell (slows us down) > JS_malloc(JSContext *cx, size_t nbytes) > { > void *p; > > JS_ASSERT(nbytes != 0); >- if (nbytes == 0) >- nbytes = 1; >+ if (nbytes <= sizeof(jsuword)) >+ nbytes = sizeof(jsuword); No need for <= just < here. The type you actually store is void *, so use that (note canonical space between typename and * :-/). >+#ifdef JS_THREADSAFE >+ JSBackgroundThread* deallocatorThread; >+ JSFreePointerListTask* deallocatorTask; Prevailing style (jorendorff and I believe this will continue to prevail) cuddles * with declarator name not type, and indents declarator (mode or name if no mode) to start on same column. Even if this style doesn't prevail in all files, it should win here in jscntxt.h and the other files that use T *p not T* p consistently. >+ inline void asynchronousFree(void* p) { >+ if (p && deallocatorTask) >+ deallocatorTask->add(p); >+ else >+ free(p); Null-defense should apply to free call too, so if (p) { if (deallocatorTask) ...; else free(p); }. >+class JSFreePointerListTask : public JSBackgroundTask { >+ void* head; >+public: >+ JSFreePointerListTask() : head(NULL) >+ { >+ } {} on previous line (one space before) is ok. >+ >+ void add(void* ptr) >+ { >+ *(void**)ptr = head; void *ptr, (void **) nits. >+ head = ptr; >+ } >+ >+ void run() >+ { These left braces can go on same line as inline method definition heads. >+ void* ptr = head; >+ while (ptr) { >+ void* next = *(void**)ptr; Nit. >+ * The Original Code is Mozilla SpiderMonkey JavaScript 1.9 code, released >+ * May 28, 2008. Use fresher date and release name (1.9.1?). >+ * >+ * The Initial Developer of the Original Code is >+ * Andreas Gal <gal@mozilla.com> >+ * >+ * Contributor(s): >+ * >+ * Alternatively, the contents of this file may be used under the terms of >+ * either of the GNU General Public License Version 2 or later (the "GPL"), >+ * or the GNU Lesser General Public License Version 2.1 or later (the "LGPL"), >+ * in which case the provisions of the GPL or the LGPL are applicable instead >+ * of those above. If you wish to allow use of your version of this file only >+ * under the terms of either the GPL or the LGPL, and not to allow others to >+ * use your version of this file under the terms of the MPL, indicate your >+ * decision by deleting the provisions above and replace them with the notice >+ * and other provisions required by the GPL or the LGPL. If you do not delete >+ * the provisions above, a recipient may use your version of this file under >+ * the terms of any one of the MPL, the GPL or the LGPL. >+ * >+ * ***** END LICENSE BLOCK ***** */ >+ >+#include "jstask.h" Grep JSTask and see the ancient name for JSRuntime, JSTaskState. Apart from Ada, "task" in the 90s meant process on some OSes. Go figure. We should prolly remove JSTaskState to shake out anyone still using it. Feel free to remove the jspubtd.h typedef in this patch. >+#ifdef JS_THREADSAFE >+static void start(void* arg) { >+ ((JSBackgroundThread*)arg)->work(); >+} What uses this? >+ >+JSBackgroundThread::JSBackgroundThread() >+{ >+ queue = NULL; >+ shutdown = NULL; >+ lock = PR_NewLock(); >+ if (!lock) >+ return; >+ sem = PR_NewCondVar(lock); >+ if (!sem) >+ return; >+ thread = PR_CreateThread(PR_USER_THREAD, start, this, PR_PRIORITY_LOW, >+ PR_LOCAL_THREAD, PR_JOINABLE_THREAD, 0); This should all go in an init method so you can OOM check. As things stand you null-defend here, and (buggily) in the dtor, but not in the methods. >+} >+ >+JSBackgroundThread::~JSBackgroundThread() >+{ >+ if (!lock) >+ return; Might want this code in a finish method to match init, with a fallible (bool with cx first param) signature. >+ if (!sem) { >+ PR_DestroyCondVar(sem); If sem is null don't crash in NSPR trying to destroy it. >+ return; >+ } >+ if (!thread) >+ return; >+ shutdown = PR_NewCondVar(lock); >+ if (!shutdown) >+ return; >+ >+ PR_Lock(lock); >+ PR_NotifyCondVar(sem); >+ PR_WaitCondVar(shutdown, PR_INTERVAL_NO_TIMEOUT); >+ PR_Unlock(lock); >+ >+ PR_DestroyCondVar(shutdown); >+ PR_DestroyCondVar(sem); >+ PR_DestroyLock(lock); Where is the JS_DestroyThread call? >+} >+ >+void >+JSBackgroundThread::work() >+{ >+ PR_Lock(lock); >+ while (true) { >+ PR_WaitCondVar(sem, PR_INTERVAL_NO_TIMEOUT); "sem" is a bit cryptic. We use names like gcDone, titleSharingDone, stateChange, etc. for other condvars. Think "what's the name of the condition" when naming a condition (variable). Here a good name might be scheduledWork or working. >+ if (shutdown) >+ break; >+ JSBackgroundTask* task; Ptr style nit. >+JSBackgroundThread::schedule(JSBackgroundTask* task) Etc. >+{ >+ PR_Lock(lock); No null defense here, but none needed with a fallible init method -- that obligates creators to check and not call further methods. >+ * The Original Code is Mozilla SpiderMonkey JavaScript 1.9 code, released >+ * May 28, 2008. Ditto earlier nit. >+class JSBackgroundTask { >+ friend class JSBackgroundThread; >+public: >+ JSBackgroundTask* next; >+public: >+ virtual void run() = 0; >+}; The first public: should be private:. >+#ifdef JS_THREADSAFE >+ >+#include "prthread.h" >+#include "prlock.h" >+#include "prcvar.h" >+ >+class JSBackgroundThread { >+ PRThread* thread; >+ JSBackgroundTask* queue; >+ PRLock* lock; >+ PRCondVar* sem; >+ PRCondVar* shutdown; Blank line here, and pointer/columnar layout style nits from jscntxt.h apply. >+public: >+ JSBackgroundThread(); >+ ~JSBackgroundThread(); >+ >+ void work(); >+ void schedule(JSBackgroundTask* task); T* p nit. Comment that schedule takes ownership of the task. Need to see OOM handling and leak-free thread joining/destroying at least. Great results though, ignoring that and the nits! ;-) /be

Sorry for the drive-by comments but the patch caught my interest: >+ if (rt->deallocatorThread) >+ rt->deallocatorTask = new JSFreePointerListTask(); > ... > + if (rt->deallocatorThread) { >+ rt->deallocatorThread->schedule(rt->deallocatorTask); I think that second if should be |if (rt->deallocatorThread && rt->deallocatorTask)| to be OOM-safe. Otherwise you'll crash in schedule(). >+JSBackgroundThread::work() > ... >+ if (shutdown) >+ break; Are you sure you want to break immediately without processing any remaining tasks in the queue? Or you could prevent tasks from being added after shutdown has begun and assert that there are no unprocessed tasks, see below. >+JSBackgroundThread::schedule(JSBackgroundTask* task) Might want to JS_ASSERT(!sem) here or return early if you've already started the shutdown on another thread. Otherwise you might schedule tasks that will never run.

The oom was due to another patch in my stack. This version address all nits by brendan and bent, except the type* vs type * in jstask.h/cpp since those are new files, and they consistently use the sane version (type*) ;)

Should be ready to go. Tested in shell and browser. Note: jstask.* still uses type* (see previous patch comment).

Also send scopes and regexp through the deallocator thread. Last patch for the night, I promise.

How is this going to interact with host object finalizers? XPConnect relies on finalizers being called during the GC pause.

asynchronousFree could do with some debug-memory-poisoning to detect memory safety errors in finalizers or perhaps slightly afterward.

The summary of this bug is not correct: we are not deferring finalization with this patch, only the underlying calls to free (which is where most of the time is anyway). Updating. What about XPConnect depends on finalizers being called there? Do we have test coverage that will inform us if we break this?

XPConnect uses internal mark and sweep with freelists based on the SM GC cycle, see: http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/src/xpcprivate.h#1517 http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/src/xpcprivate.h#1650 and others... Then unmarked classes are freed from the gc callback here: http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/src/xpcjsruntime.cpp#119 called from http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/src/xpcjsruntime.cpp#641 I'm pretty sure most of this is guarded by assertions and would also crash pretty quickly in our various tests, but I don't think there are tests specifically for this behavior.

> Do we have test coverage that will inform us if we break this? With fatal assertions, yes. The moment you try to finalize an XPCWrappedNative for a window you'll QI the underlying nsGlobalWindow and should assert in its non-threadsafe QI.

That would be the case if we finalized on another thread, but what if we just finalized out from under the GC lock?

Oh, I see what you meat by "there". So yeah, comment 15 is it.

I am not sure I understand the discussion here. We merely postpone the free() for certain items that we allocated inside the JS engine with malloc. We do not postpone or delay or muck with whatever internal nightmares xpconnect does to its own strings. Those gc finalizer calls still occur from within the GC lock and on the main thread. With this patch, we merely change the behavior of free() to free a little later.

(In reply to comment #19) > I am not sure I understand the discussion here. I probably caused the confusion: when I filed the bug, I put "delay finalizers" in the title, but of course I should have said "delay |free|".

Comment on attachment 389899 [details] [diff] [review] patch >+JSBackgroundThread::init() > ... >+ thread = PR_CreateThread(PR_USER_THREAD, start, this, PR_PRIORITY_LOW, >+ PR_LOCAL_THREAD, PR_JOINABLE_THREAD, 0); You're making a joinable thread but never joining with it. Instead you're doing something similar with the |shutdown| condvar. I think you should make |shutdown| a simple boolean and then change cancel like so: JSBackgroundThread::cancel() { PR_Lock(lock); if (shutdown) return; shutdown = JS_TRUE; PR_NotifyCondVar(wakeup); PR_Unlock(lock); PR_JoinThread(thread); } >+JSBackgroundThread::work() I noticed that you're holding onto the lock throught the entire free'ing loop. Couldn't that make the main thread block more often than you'd really like? What about locking only long enough to grab the queue, assign it to a stack var, and then unlocking to process it?

Or use one of those fancy lock-free queues.

I will actually likely do the opposite and make it less concurrent. We don't want the deallocator thread to fall behind too far under memory pressure.

(In reply to comment #23) > I will actually likely do the opposite and make it less concurrent. We don't > want the deallocator thread to fall behind too far under memory pressure. Don't hold a lock for long, it could be a spinlock. If you really want to make the main thread wait on the free-er thread, you'd want a condvar. But falling behind should be detectable by the main thread, and it should free in the foreground -- this is what we talked about at lunch yesterday, and it seems necessary and sufficient. /be

Yeah, I will do the latter. The main thread will switch over the foreground deallocation and let the background thread catch up.

Comment on attachment 390287 [details] [diff] [review] patch >+JSBackgroundThread::schedule(JSBackgroundTask* task) >+{ >+ PR_Lock(lock); >+ if (shutdown) { >+ task->run(); You should delete task after running here to avoid leaking, and i think you might want to run the task outside the lock. r=me with that!

Comment on attachment 390317 [details] [diff] [review] patch >+JSBackgroundThread::schedule(JSBackgroundTask* task) >+{ >+ PR_Lock(lock); >+ if (shutdown) { >+ PR_Unlock(lock); >+ task->run(); >+ delete task; >+ return; >+ } else { else after return non-sequitur. >+ task->next = queue; >+ queue = task; queue is a misnomer, this is task stack! Is the LIFO nature of "queue" a potential problem? Late bike-shedding idea: s/task/job/ -- take it or leave it. >+class JSBackgroundThread { >+ PRThread* thread; >+ JSBackgroundTask* queue; >+ PRLock* lock; >+ PRCondVar* wakeup; >+ bool shutdown; >+ >+ public: For the ealier class (the pointer-freeing class derived from JSBackgroundTask) you did not indent public: by two spaces, here you do. Prevailing style says to indent as here, so fix the other case. /be

dmandelin suggests gig, r=me on that name

Comment on attachment 390317 [details] [diff] [review] patch Didn't get a final patch so +'ing here with changes. /be

Argh. Merge conflict lost the most important JS_free. http://hg.mozilla.org/tracemonkey/rev/53b153375585

Hm, I forgot that we don't know extent of the pointer inside asynchronousFree, but there is a sane default, and the caller can make an accurate lower-bound estimate of that extent if possible. I would assume, so long as that estimate isn't used if !DEBUG, providing the estimate shouldn't result in any extra code being generated by modern compilers. Please add this poisoning to the next version of the patch.

Wait, don't JSFreePointerListTask::add and JSFreePointerListTask::run need to be threadsafe? JSRuntime::deallocatorTask is MT; shouldn't it rather be JSContext::deallocatorTask? Also, a nit: my fingers are continually typing asyncFree rather than asynchronousFree when trying to use find-as-you-type on the patch diffs. I would prefer this shorter naming both to ease my mental discomfort and to save typing for everyone.

Wait, GC only occurs on one thread at a time, never mind.

(In reply to comment #37) > Wait, GC only occurs on one thread at a time, never mind. But your asyncFree comment was on target -- agreed. Andreas, please lose the overlong "hronous". /be

(In reply to comment #35) > Hm, I forgot that we don't know extent of the pointer inside asynchronousFree, > but there is a sane default, and the caller can make an accurate lower-bound > estimate of that extent if possible. I would assume, so long as that estimate > isn't used if !DEBUG, providing the estimate shouldn't result in any extra code > being generated by modern compilers. Please add this poisoning to the next > version of the patch. The *(void **)ptr = next store is the only poisoning that looks feasible (in this case, inevitable). We don't require allocations freed this way to be > sizeof(void *) -- it's a deferred free call. /be

Those of us not logging IRC could use a play-by-play on where the patch stands, why it bounced off tm tinderbox, etc. Thanks, /be

Those of trying to get the patch landed would love the answers for #41 as well :) It bounces of tinderboxes on anything but mac. Maybe I am misusing NSPR somehow. Or jemalloc hates me. I don't know. I will play around with a windows build today and see if I can make it crash.

Are you sure ptr always points to enough memory to stuff a void* in that memory? I don't know about the Mac allocator, but jemalloc will hand out 2-byte allocations if you ask for them...

Vlad assured me that jemalloc only ever gave back 16 bytes or more! That could indeed be the problem.

JS_malloc() pads to sizeof(void*) if you allocate less (see patch).

(In reply to comment #43) > I don't know about the Mac allocator, but jemalloc will hand out > 2-byte allocations if you ask for them... Mac malloc() always returns a 16-byte aligned block... not sure about the minimum size.

Interesting! 50% of our allocations on SS are 4 and 6 bytes.

If anyone wants to test the build, here is a link: https://build.mozilla.org/tryserver-builds/agal@mozilla.com-try-96c93e03116f/ MacOSX is known to work (I am using it). Linux and Windows crash tinderbox.

> Vlad assured me that jemalloc only ever gave back 16 bytes or more! That doesn't seem to match http://mxr.mozilla.org/mozilla-central/source/memory/jemalloc/jemalloc.c#53 or the "#define TINY_MIN_2POW 1" in that file.

(In reply to comment #40) > The *(void **)ptr = next store is the only poisoning that looks feasible (in > this case, inevitable). We don't require allocations freed this way to be > > sizeof(void *) -- it's a deferred free call. You misunderstand, but perhaps it's better for me to write the patch I meant as a followup and not distract from figuring out the base problem here.

(In reply to comment #50) > (In reply to comment #40) > > The *(void **)ptr = next store is the only poisoning that looks feasible (in > > this case, inevitable). We don't require allocations freed this way to be > > > sizeof(void *) -- it's a deferred free call. > > You misunderstand, The burden really is on you to make yourself clear :-/. > but perhaps it's better for me to write the patch I meant as > a followup and not distract from figuring out the base problem here. Ok. /be

This patch uses Malloc/Realloc/Calloc/Free all over the source base, and introduces requivalent runtime-> and context-> memory allocation/deallocation functions. Its a big patch, but this guarantees that all allocations are forced >= sizeof(void*). With that we can now pipe all frees through a thread-local deallocator thread. Currently its still only enabled during GC, but that could change int he future.

Do we really need to use "asyncSafeMalloc" as the static inline jsutil.h function name? It would be more future-proof to have what embedders have long wanted (can't find the bug right now): js_malloc -> ::malloc js_free -> ::free js_calloc -> ::calloc js_realloc -> ::realloc These could go in jsutil.h, that's not a bad place. But the names match prevailing collision-avoidance style and do not force capitalization on the old Unix names (malloc not Malloc!). /be

why do you want all < sizeof(void*) allocations to be sizeof(void*)?(In reply to comment #44) > Vlad assured me that jemalloc only ever gave back 16 bytes or more! That could > indeed be the problem. There is a nice chart http://mxr.mozilla.org/mozilla-central/source/memory/jemalloc/jemalloc.c#53 that will tell you exactly what jemalloc will give you! ;)

nevermind on the sizeof(void*) thing

The < 2 case just got eliminated, but its pretty rare for the JS engine anyway.

timeless noticed a missing return p; in JSContext::calloc(), new patch attached.

Note to self: timeless also reports whitespace/trailing \ alignment issues. Have to check that before committing.

@@ -396,17 +396,17 @@ JSScope::changeTable(JSContext *cx, int i think this should be cx->calloc: + newtable = (JSScopeProperty **) js_calloc(nbytes); JS_ReportOutOfMemory(cx); + cx->free(oldtable); @@ -3602,17 +3602,17 @@ TraceRecorder::compile(JSTraceMonitor* t const char* filename = cx->fp->script->filename; char* label = (char*)malloc((filename ? strlen(filename) : 7) + 16); you missed this, it should be cx->malloc - free(label); + js_free(label); and this it the pair, which should be cx->free XMLArray* generally have a cx pointer available, see here: XMLArraySetCapacity(JSContext *cx, JSXMLArray *array, uint32 capacity) + js_free(array->vector); + js_realloc(array->vector, capacity * sizeof(void *)))) { if (cx) JS_ReportOutOfMemory(cx); i claim that they should be cx->... XMLArrayFinish(JSContext *cx, JSXMLArray *array) + cx->free(array->vector); especially given that... @@ -1034,17 +1034,17 @@ XMLArrayAddMember(JSContext *cx, JSXMLAr + js_realloc(array->vector, capacity * sizeof(void *)))) { JS_ReportOutOfMemory(cx); @@ -1115,20 +1115,20 @@ XMLArrayTruncate(JSContext *cx, JSXMLArr + js_free(array->vector); + vector = (void **) js_realloc(array->vector, length * sizeof(void *)); i'm not sure about explicit checks against null: + while ((task = stack) != NULL) { would you mind using one line per variable here: + : thread(NULL), stack(NULL), lock(NULL), wakeup(NULL), shutdown(false) whitespace: - ptr_ = JS_realloc(cx, stackbuf, (stackmax+1) * sizeof(jschar)); \ + ptr_ = cx->realloc(stackbuf, (stackmax+1) * sizeof(jschar)); \ (twice) could you add some whitespace around - and + ?, + tmp = cx->malloc(j-i+2); + result = cx->malloc(strlen(dir)+1+strlen(tmp)+1); whitespace for these second lines need more indentation: + tmp = cx->realloc(buf, (offset + MAX_LINE_LENGTH) * sizeof data); + array = (void **) js_realloc(table->array, capacity * sizeof table->array[0]); - JS_malloc(cx, sizeof(JSGenerator) + (nslots - 1) * sizeof(jsval)); + cx->malloc(sizeof(JSGenerator) + (nslots - 1) * sizeof(jsval)); 3 space instead of 4: +public: + JSFreePointerListTask() : head(NULL) {}

The patch passes tryserver on all platforms. Ready to land as soon the final review is done.

Comment on attachment 390750 [details] [diff] [review] patch with formating nits fixed >diff --git a/js/src/jscntxt.h b/js/src/jscntxt.h >+ inline void* malloc(size_t bytes) { >+ JS_ASSERT(bytes != 0); >+ void *p = runtime->malloc(bytes); >+ if (!p) { >+ JS_ReportOutOfMemory(this); >+ return NULL; >+ } >+ updateMallocCounter(bytes); >+ return p; >+ } >+ >+ inline void* calloc(size_t bytes) { >+ JS_ASSERT(bytes != 0); >+ void *p = runtime->calloc(bytes); >+ if (!p) { >+ JS_ReportOutOfMemory(this); >+ return NULL; >+ } >+ updateMallocCounter(bytes); >+ return p; >+ } >+ >+ inline void* realloc(void* p, size_t bytes) { >+ void *orig = p; >+ p = runtime->realloc(p, bytes); >+ if (!p) { >+ JS_ReportOutOfMemory(this); >+ return NULL; >+ } >+ if (!orig) >+ updateMallocCounter(bytes); >+ return p; >+ } >+ >+#ifdef JS_THREADSAFE >+ inline void createDeallocatorTask() { >+ JSThreadData* tls = JS_THREAD_DATA(this); >+ JS_ASSERT(!tls->deallocatorTask); >+ if (runtime->deallocatorThread && !runtime->deallocatorThread->busy()) >+ tls->deallocatorTask = new JSFreePointerListTask(); >+ } >+ >+ inline void submitDeallocatorTask() { >+ JSThreadData* tls = JS_THREAD_DATA(this); >+ if (tls->deallocatorTask) { >+ runtime->deallocatorThread->schedule(tls->deallocatorTask); >+ tls->deallocatorTask = NULL; >+ } >+ } >+ >+ inline void free(void* p) { >+ if (!p) >+ return; >+ if (thread) { >+ JSFreePointerListTask* task = JS_THREAD_DATA(this)->deallocatorTask; >+ if (task) { >+ task->add(p); >+ return; >+ } >+ } >+ runtime->free(p); >+ } >+#else >+ inline void free(void* p) { >+ if (!p) >+ return; >+ runtime->free(p); >+ } >+#endif > }; The free() definitions here should be immediately after the malloc/calloc/realloc definitions. The deallocator-task threadsafe-only definitions should precede malloc/calloc/realloc/free, then, in its own #ifdef JS_THREADSAFE / #endif section. >diff --git a/js/src/jsexn.cpp b/js/src/jsexn.cpp >@@ -581,17 +581,17 @@ StackTraceToString(JSContext *cx, JSExnP > > #define APPEND_CHAR_TO_STACK(c) \ > JS_BEGIN_MACRO \ > if (stacklen == stackmax) { \ > void *ptr_; \ > if (stackmax >= STACK_LENGTH_LIMIT) \ > goto done; \ > stackmax = stackmax ? 2 * stackmax : 64; \ >- ptr_ = JS_realloc(cx, stackbuf, (stackmax+1) * sizeof(jschar)); \ >+ ptr_ = cx->realloc(stackbuf, (stackmax+1) * sizeof(jschar)); \ Misplaced \ here. >@@ -603,17 +603,17 @@ StackTraceToString(JSContext *cx, JSExnP > str_->getCharsAndLength(chars_, length_); \ > if (length_ > stackmax - stacklen) { \ > void *ptr_; \ > if (stackmax >= STACK_LENGTH_LIMIT || \ > length_ >= STACK_LENGTH_LIMIT - stacklen) { \ > goto done; \ > } \ > stackmax = JS_BIT(JS_CeilingLog2(stacklen + length_)); \ >- ptr_ = JS_realloc(cx, stackbuf, (stackmax+1) * sizeof(jschar)); \ >+ ptr_ = cx->realloc(stackbuf, (stackmax+1) * sizeof(jschar)); \ And here. >diff --git a/js/src/jsfile.cpp b/js/src/jsfile.cpp 25K of changes here, none of which I reviewed. You could have replaced every statement with cx->useUnicornsAndPixieDust() and I wouldn't know! :-( Can we please please please delete this yesterday?</rant-not-substantive-comment-to-be-acted-upon-here> >diff --git a/js/src/jsgc.cpp b/js/src/jsgc.cpp >@@ -751,17 +752,17 @@ AddToPtrTable(JSContext *cx, JSPtrTable > */ > JS_STATIC_ASSERT(2 <= sizeof table->array[0]); > capacity = (capacity < info->linearGrowthThreshold) > ? 2 * capacity > : capacity + info->linearGrowthThreshold; > if (capacity > (size_t)-1 / sizeof table->array[0]) > goto bad; > } >- array = (void **) realloc(table->array, >+ array = (void **) js_realloc(table->array, > capacity * sizeof table->array[0]); Need to realign the second line here. >diff --git a/js/src/jsgc.h b/js/src/jsgc.h >+#ifdef JS_THREADSAFE >+class JSFreePointerListTask : public JSBackgroundTask { >+ void *head; >+ public: >+ JSFreePointerListTask() : head(NULL) {} >+ >+ void add(void* ptr) { >+ *(void**)ptr = head; >+ head = ptr; >+ } >+ >+ void run() { >+ void *ptr = head; >+ while (ptr) { >+ void *next = *(void **)ptr; >+ free(ptr); >+ ptr = next; >+ } >+ } >+}; I think it would be slightly clearer, stylistically, if free were js_free here. :-\ I hate allocator matching. >diff --git a/js/src/jsobj.cpp b/js/src/jsobj.cpp >@@ -699,30 +699,30 @@ obj_toSource(JSContext *cx, uintN argc, > #endif > goto make_string; > } > JS_ASSERT(ida); > ok = JS_TRUE; > > if (!chars) { > /* If outermost, allocate 4 + 1 for "({})" and the terminator. */ >- chars = (jschar *) malloc(((outermost ? 4 : 2) + 1) * sizeof(jschar)); >+ chars = (jschar *) js_malloc(((outermost ? 4 : 2) + 1) * sizeof(jschar)); > nchars = 0; > if (!chars) > goto error; > if (outermost) > chars[nchars++] = '('; > } else { > /* js_EnterSharpObject returned a string of the form "#n=" in chars. */ > MAKE_SHARP(he); > nchars = js_strlen(chars); > chars = (jschar *) >- realloc((ochars = chars), (nchars + 2 + 1) * sizeof(jschar)); >+ js_realloc((ochars = chars), (nchars + 2 + 1) * sizeof(jschar)); > if (!chars) { >- free(ochars); >+ js_free(ochars); > goto error; > } > if (outermost) { > /* > * No need for parentheses around the whole shebang, because #n= > * unambiguously begins an object initializer, and never a block > * statement. > */ >@@ -953,21 +953,21 @@ obj_toSource(JSContext *cx, uintN argc, > SAFE_ADD((outermost ? 2 : 1) + 1); > #undef SAFE_ADD > > if (curlen > (size_t)-1 / sizeof(jschar)) > goto overflow; > > /* Allocate 1 + 1 at end for closing brace and terminating 0. */ > chars = (jschar *) >- realloc((ochars = chars), curlen * sizeof(jschar)); >+ js_realloc((ochars = chars), curlen * sizeof(jschar)); > if (!chars) { > /* Save code space on error: let JS_free ignore null vsharp. */ >- JS_free(cx, vsharp); >- free(ochars); >+ cx->free(vsharp); >+ js_free(ochars); > goto error; > } > > if (comma) { > chars[nchars++] = comma[0]; > chars[nchars++] = comma[1]; > } > comma = ", "; >@@ -1000,55 +1000,55 @@ obj_toSource(JSContext *cx, uintN argc, > if (vsharplength) { > js_strncpy(&chars[nchars], vsharp, vsharplength); > nchars += vsharplength; > } > js_strncpy(&chars[nchars], vchars, vlength); > nchars += vlength; > > if (vsharp) >- JS_free(cx, vsharp); >+ cx->free(vsharp); > } > } > > chars[nchars++] = '}'; > if (outermost) > chars[nchars++] = ')'; > chars[nchars] = 0; > > error: > js_LeaveSharpObject(cx, &ida); > > if (!ok) { > if (chars) >- free(chars); >+ js_free(chars); > goto out; > } > > if (!chars) { > JS_ReportOutOfMemory(cx); > ok = JS_FALSE; > goto out; > } > make_string: > str = js_NewString(cx, chars, nchars); > if (!str) { >- free(chars); >+ js_free(chars); > ok = JS_FALSE; > goto out; > } > *vp = STRING_TO_JSVAL(str); > ok = JS_TRUE; > out: > JS_POP_TEMP_ROOT(cx, &tvr); > return ok; > > overflow: >- JS_free(cx, vsharp); >- free(chars); >+ cx->free(vsharp); >+ js_free(chars); > chars = NULL; > goto error; > } > #endif /* JS_HAS_TOSOURCE */ This method's passed a cx, so why doesn't all of it use cx->malloc/free? >diff --git a/js/src/json.cpp b/js/src/json.cpp >@@ -735,17 +735,17 @@ js_BeginJSONParse(JSContext *cx, jsval * > { > if (!cx) > return NULL; > > JSObject *arr = js_NewArrayObject(cx, 0, NULL); > if (!arr) > return NULL; > >- JSONParser *jp = (JSONParser*) JS_malloc(cx, sizeof(JSONParser)); >+ JSONParser *jp = (JSONParser*) cx->malloc(sizeof(JSONParser)); > if (!jp) > return NULL; > memset(jp, 0, sizeof *jp); This obviously should be cx->calloc. Fewer lines ftw! >diff --git a/js/src/jsregexp.cpp b/js/src/jsregexp.cpp > morepar = (JSSubString*) >- JS_realloc(cx, morepar, >+ cx->realloc(morepar, > res->moreLength * sizeof(JSSubString)); Misalignment again. >diff --git a/js/src/jsstr.cpp b/js/src/jsstr.cpp >@@ -2616,35 +2616,34 @@ js_GetUnitStringForChar(JSContext *cx, j > { > jschar *cp, i; > JSRuntime *rt; > JSString **sp; > > JS_ASSERT(c < UNIT_STRING_LIMIT); > rt = cx->runtime; > if (!rt->unitStrings) { >- sp = (JSString **) calloc(UNIT_STRING_LIMIT * sizeof(JSString *) + >- UNIT_STRING_LIMIT * 2 * sizeof(jschar), >- 1); >+ sp = (JSString **) js_calloc(UNIT_STRING_LIMIT * sizeof(JSString *) + >+ UNIT_STRING_LIMIT * 2 * sizeof(jschar)); Second line misindented. >@@ -3161,27 +3160,27 @@ js_DeflateString(JSContext *cx, const js > #ifdef DEBUG > JSBool ok; > #endif > > if (js_CStringsAreUTF8) { > nbytes = js_GetDeflatedStringLength(cx, chars, nchars); > if (nbytes == (size_t) -1) > return NULL; >- bytes = (char *) (cx ? JS_malloc(cx, nbytes + 1) : malloc(nbytes + 1)); >+ bytes = (char *) (cx ? cx->malloc(nbytes + 1) : js_malloc(nbytes + 1)); > if (!bytes) > return NULL; > #ifdef DEBUG > ok = > #endif > js_DeflateStringToBuffer(cx, chars, nchars, bytes, &nbytes); > JS_ASSERT(ok); > } else { > nbytes = nchars; >- bytes = (char *) (cx ? JS_malloc(cx, nbytes + 1) : malloc(nbytes + 1)); >+ bytes = (char *) (cx ? cx->malloc(nbytes + 1) : js_malloc(nbytes + 1)); > if (!bytes) > return NULL; > for (i = 0; i < nbytes; i++) > bytes[i] = (char) chars[i]; > } > bytes[nbytes] = 0; > return bytes; > } Ugh, impossible-to-avoid OOM-reporting fail. :-( >@@ -4831,17 +4830,17 @@ AddCharsToURI(JSContext *cx, JSCharBuffe > { > size_t total; > jschar *newchars; > > total = buf->length + length + 1; > if (!buf->chars || > JS_HOWMANY(total, URI_CHUNK) > JS_HOWMANY(buf->length + 1, URI_CHUNK)) { > total = JS_ROUNDUP(total, URI_CHUNK); >- newchars = (jschar *) JS_realloc(cx, buf->chars, >+ newchars = (jschar *) cx->realloc(buf->chars, > total * sizeof(jschar)); Misalignment. >diff --git a/js/src/jstask.cpp b/js/src/jstask.cpp >+ * The Initial Developer of the Original Code is >+ * Andreas Gal <gal@mozilla.com> >+ * >+ * Contributor(s): >+ * Mozilla Mumble-Mumble is the initial developer, you should list yourself in the Contributors section (typically with a (original author) note afterward, but it's not required). >+#ifdef JS_THREADSAFE >+static void start(void* arg) { >+ ((JSBackgroundThread*)arg)->work(); >+} Make this a private static class method: void JSBackgroundThread::start(void* arg). >+void >+JSBackgroundThread::work() >+{ >+ PR_Lock(lock); >+ do { >+ PR_WaitCondVar(wakeup, PR_INTERVAL_NO_TIMEOUT); >+ JSBackgroundTask* task; >+ while ((task = stack) != NULL) { >+ stack = task->next; >+ PR_Unlock(lock); >+ task->run(); >+ delete task; >+ PR_Lock(lock); >+ } >+ } while (!shutdown); >+ PR_Unlock(lock); >+} I think |task| (or |startingTask| if you'd rather not rename the |task| local to something like |t|) would be a more descriptive (and more accurate) name for the task list member. (JSFreePointerListTask::next then implies the queue-ness of the overall task list.) >diff --git a/js/src/jstask.h b/js/src/jstask.h >+ * The Initial Developer of the Original Code is >+ * Andreas Gal <gal@mozilla.com> >+ * >+ * Contributor(s): >+ * Same comment as above. >diff --git a/js/src/jsxml.cpp b/js/src/jsxml.cpp >@@ -928,25 +928,25 @@ XMLArrayCursorTrace(JSTracer *trc, JSXML > static JSBool > XMLArraySetCapacity(JSContext *cx, JSXMLArray *array, uint32 capacity) > { > void **vector; > > if (capacity == 0) { > /* We could let realloc(p, 0) free this, but purify gets confused. */ > if (array->vector) >- free(array->vector); >+ js_free(array->vector); This could be cx->free, no? > vector = NULL; > } else { > if ( > #if JS_BITS_PER_WORD == 32 > (size_t)capacity > ~(size_t)0 / sizeof(void *) || > #endif > !(vector = (void **) >- realloc(array->vector, capacity * sizeof(void *)))) { >+ js_realloc(array->vector, capacity * sizeof(void *)))) { cx->realloc? >@@ -1034,17 +1034,17 @@ XMLArrayAddMember(JSContext *cx, JSXMLAr > JS_CEILING_LOG2(log2, capacity); > capacity = JS_BIT(log2); > } > if ( > #if JS_BITS_PER_WORD == 32 > (size_t)capacity > ~(size_t)0 / sizeof(void *) || > #endif > !(vector = (void **) >- realloc(array->vector, capacity * sizeof(void *)))) { >+ js_realloc(array->vector, capacity * sizeof(void *)))) { cx->realloc? >@@ -1115,20 +1115,20 @@ XMLArrayTruncate(JSContext *cx, JSXMLArr > void **vector; > > JS_ASSERT(!array->cursors); > if (length >= array->length) > return; > > if (length == 0) { > if (array->vector) >- free(array->vector); >+ js_free(array->vector); cx->free? > vector = NULL; > } else { >- vector = (void **) realloc(array->vector, length * sizeof(void *)); >+ vector = (void **) js_realloc(array->vector, length * sizeof(void *)); cx->free? >@@ -2154,17 +2154,17 @@ MakeXMLSpecialString(JSContext *cx, JSSt > js_strncpy(bp, str2->chars(), length2); > bp += length2; > } > js_strncpy(bp, suffix, suffixlength); > bp[suffixlength] = 0; > > str = js_NewString(cx, base, newlength); > if (!str) >- free(base); >+ js_free(base); cx->free? >@@ -2205,17 +2205,17 @@ MakeXMLPIString(JSContext *cx, JSStringB > */ > static void > AppendAttributeValue(JSContext *cx, JSStringBuffer *sb, JSString *valstr) > { > js_AppendChar(sb, '='); > valstr = js_EscapeAttributeValue(cx, valstr, JS_TRUE); > if (!valstr) { > if (STRING_BUFFER_OK(sb)) { >- free(sb->base); >+ js_free(sb->base); cx->free?

(In reply to comment #71) > > vector = NULL; > > } else { > >- vector = (void **) realloc(array->vector, length * sizeof(void *)); > >+ vector = (void **) js_realloc(array->vector, length * sizeof(void *)); > > cx->free? Er. You know what I mean. :-)

I didn't change any reallocs that previously didn't go via JS_realloc, same for malloc, since I didn't want to introduce new error reporting where there wasn't any before. Thats for a follow-up patch. I did fix the free -> cx->free things since those are just empty wrappers without behavioral differences.

I seem to have regressed the talos SS numbers? Could someone confirm that?

I get mail about it if you do. Don't see any so far.

02:12:41 <@jorendorff> deadlock on exit, thread 1 is in main ->JS_Finish->cancel->PR_JoinThread 02:12:52 <@jorendorff> thread 2 in JSBackgroundThread::work -> PR_WaitCondVar jorendorff says its about 4% likelihood and it happens when the browser shuts down (linux).

I am definitively seeing some lock contention (at least on macosx) with this patch in the malloc path on synthetic benchmarks. Not visible on real workloads. Either way, I will follow up with a per-thread pool allocator to compensate for the lock contention. jemalloc probably does a lot better with this than macosx malloc.

Mass change: adding fixed1.9.2 keyword (This bug was identified as a mozilla1.9.2 blocker which was fixed before the mozilla-1.9.2 repository was branched (August 13th, 2009) as per this query: http://is.gd/2ydcb - if this bug is not actually fixed on mozilla1.9.2, please remove the keyword. Apologies for the bugspam)