123584 - JavaScript engine should use malloc/realloc/free consistently

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brent Thomopson]

Currently within the JavaScript (SpiderMonkey) source, there seems to be 3 
separate ways that memory allocations are managed.  Sometimes, the internal 
wrapper functions JS_malloc()/JS_realloc()/JS_free() are used.  At other times, 
the macros JS_MALLOC()/JS_DELETE()/JS_NEW() are used.  And still other times, 
the host system malloc()/realloc()/free() are used directly.  

I would propose that the the wrapper functions JS_malloc()/JS_realloc()/JS_free
() be used throughout the source code.  

In the embedded VxWorks system I am working in, we are trying to carve out a 
separate memory pool for the JavaScript engine to play in, and having 
SpiderMonkey go through wrapper functions (or even macros) for all memory 
allocations would greatly help centralize the code changes needed to work in 
this environment.  Thanks!

Comment 1

[Phil Schwartau]

cc'ing Brendan on this one - 

Comment 2

[Brendan Eich [:brendan]]

JS (and for that matter most of Mozilla) doesn't work unless you use the one
true malloc (whichever one that is, it must be the only one).  Mozilla is far
from a fix here, and scouring the code to balance wrappers is a non-goal for
mozilla1.0.  In fact the over-layered XPCOM allocator service, combined with too
many mallocs called through it, are a time performance problem.

Anyway (sorry to digress), even for JS, I never intended JS_malloc to be other
than a wrapper for malloc *that reports OOM errors using cx and JS_ReportError*.
 That's the value it adds, not the ability to substitute something else for
malloc.  Longstanding code in the engine explicitly wants to be able to call
free on memory returned by JS_malloc, and to malloc storage passed to JS_free,
although symmetry rarely breaks (the breaks are usually malloc/JS_free, in order
to consolidate error reporting).  This could all be "fixed" at some negligable
cost in cycles, and non-negligable cost in hacking effort.

The JS_NEW and JS_MALLOC macros in jsutil.h snuck in when fur cloned some NSPR2
files to make JS stand alone, when he led the charge to kill JSRef (my old and
separated-source standalone engine, from which the NSPR-based js/src code used
to be derived via sed).  No one uses those macros, AFAICT.  JS_DELETE is used,
inconsistently as you noted, in jsprf.c, a fork of NSPR's prprf.c.

This is all ugly, but I have no time to clean it up now.  I suspect Kenton also
has bigger fish to fry.  But patches are welcome!  Please help yourself, and
we'll help get any that are good to go in, even during mozilla0.9.9 (mozilla1.0
is another matter, but worth a try if you can't make 0.9.9).  I hope js1.5 is
declared done at the same time mozilla1.0 "ships".

/be

Comment 3

[timeless]

Attachment: remove JS_DELETE

Comment 4

[timeless]

Comment on attachment 113685 [details] [diff] [review]
remove JS_DELETE

the non -w patch changes a lot of whitespace ...

Comment 5

[timeless]

.

Comment 6

[Brendan Eich [:brendan]]

I was gonna review+, but rogerl did, so I'll take the liberty of encouraging
timeless to check this in as soon as 1.4 opens.

/be

Comment 7

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

hm, disclaimer: I don't know this stuff well, sorry if I annoy you

but two comments:
 	jsdebugstr(buffer);
-	JS_DELETE(buffer);
+    JS_smprintf_free(buffer);
shouldn't you be consistent with the whitespace above?

and, should the JS_DELETE define be removed? comment 2 sounds to me like it's
not needed.

Comment 8

[Brendan Eich [:brendan]]

biesi: timeless fixed overindentation everywhere in jsutil.c, but it didn't show
up in the diff -w.  But yeah, why not nuke JS_DELETE entirely?

/be

Comment 9

[timeless]

I wasn't sure about changing apis :), i'll gladly remove the JS_DELETE define
when i commit after 1.4 opens.

Comment 10

[Nicholas Allen]

timeless- are you still planning to nuke these 3 macros?

Comment 11

[timeless]

checked in.
i meant to comment earlier...
reporter: are you still trying to carve out a memory pool for spidermonkey? you
might try to start a carving for just spidermonkey arenas (i have a patch
somewhere which tries to do it for w32, although it doesn't work..)