Closed Bug 443500 Opened 12 years ago Closed 11 years ago
Make nanojit use explicit allocation/deallocation (optionally).
22.00 KB, text/plain
1.92 KB, text/plain
49.57 KB, patch
|Details | Diff | Splinter Review|
For uses of nanojit outside Tamarin it would be nice to not have any dependencies on MMgc. Also, since MMgc is not thread safe, for certain parallel optimizations and benchmarking uses a GC-free nanojit would be useful. Last but not least, even for MMgc + nanojit explicit deallocation where possible would be a win (reduced overall memory footprint through eager/early deallocation). I will attach our current GC-free avmplus.h environment. Jungwoo will pick and chose from our patch and his patch and post a proposed patch for Tamarin.
+1 although I'd remove the "optional" bit, these ideas are all good in general.
Attached patch runs on tamarin tracing. I tested on Windows, MacOSX, Linux. In avmbuild.h there is a flag FEATURE_NANOJIT_NOGC that strips out MMgc from nanojit.
I think we should further split up the patch in at least 2 parts: 1. Add deletes to the code as is (Ed wants explicit deletes via the GC, even if MMgc is enabled). 2. Mimic the GC interface for alloc/dealloc if we don't have a real GC. All the #ifdefs are a maintainance hazard and its difficult to stick to this style.
+1 something like #ifdef FEATURE_NANOJIT_NOGC typedef struct NanoGC_* NanoGC; // opaque type #else typedef GC* NanoGC; // opaque type #endif would do the trick, and the NOGC versions could just ignore it as an argument.
Sounds good. It would do the trick if we are to remove all MMgc from the VM implementation including interpreters. However, you have to note that in some place, both NanoGC_ and GC object need to be together in TT. For example, Filters used in Interpreter.cpp needs GC, and Fragment uses NanoGC_. Thus ifdefs cannot be cleanly removed in such places. Any suggestion on this matter?
I think there was talk about making filters in the forward path (Interpreter) stack allocated. Ed?
yes, i would like to do that. should we make it a prerequisite for this bug?
I guess we can keep working on them in parallel. Jungwoo can just ignore all the GC allocations that happen in Interpreter and just make nanojit GC-free.
Is anyone working on thus bug atm?
Jungwoo, are you still working on this bug?
Can you tell me more specific on what you expect? My nogc code is currently tightly coupled with to enable parallel jit that I'm working on. I can cleanup the code and submit, but it would be different from what you exepct. For example, LabelNameMap uses String internally, and I didn't make this GC free.
We are very interested in both features, no GC and parallel jit-ing. Only the non-debug code has to be leak-free. For LabelNameMap I am ok with leaking memory for now.
So that means you want this patch to be leak free, right? That was not in my attention. I took care of new and malloc, but not delete and free. If it doesn't need to be strictly leak-free, I can submit the patch.
Explicit allocation is great, but we will also need explicit de-allocation. Maybe we should split this bug to unblock each other and we will work separately on the GC-issue and parallelism.
Created a new bug for the parallel compilation portion of this feature and assigned to Jungwoo (https://bugzilla.mozilla.org/show_bug.cgi?id=449865). Taking over the explicit alloc/dealloc feature (this bug).
Assignee: jungwoo.ha → gal
We have a patch in the works. We will land it in TM and then queue it for review for Tamarin after a bit of testing.
patch against tracemonkey, passes valgrind on trace.js (cannot run trace-tests.js, iloops in valgrind no matter what). this is not tested in TT yet
Attachment #333294 - Flags: review?(gal)
Comment on attachment 333294 [details] [diff] [review] explicit deallocation for fragments and friends Looks good as temporary stop-gap measure. Lets land it in TM.
Attachment #333294 - Flags: review?(gal) → review+
Comment on attachment 333294 [details] [diff] [review] explicit deallocation for fragments and friends Pushed.
Attachment #333294 - Attachment is obsolete: true
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.