Closed Bug 123584 Opened 23 years ago Closed 20 years ago

JavaScript engine should use malloc/realloc/free consistently

Categories

(Core :: JavaScript Engine, defect)

All
Other
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.7beta

People

(Reporter: thompson, Assigned: timeless)

Details

Attachments

(1 file)

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!
cc'ing Brendan on this one - 
Assignee: rogerl → khanson
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: JavaScript engine should use malloc/realloc/free consistantly → JavaScript engine should use malloc/realloc/free consistently
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
Target Milestone: --- → Future
Attached patch remove JS_DELETESplinter Review
Comment on attachment 113685 [details] [diff] [review]
remove JS_DELETE

the non -w patch changes a lot of whitespace ...
Attachment #113685 - Flags: review?(rogerl)
.
Assignee: khanson → timeless
Target Milestone: Future → ---
Attachment #113685 - Flags: review?(rogerl) → review+
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
Target Milestone: --- → mozilla1.4alpha
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.
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
I wasn't sure about changing apis :), i'll gladly remove the JS_DELETE define
when i commit after 1.4 opens.
Status: NEW → ASSIGNED
timeless- are you still planning to nuke these 3 macros?
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..)
Target Milestone: mozilla1.4alpha → mozilla1.7beta
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: