443500 - Make nanojit use explicit allocation/deallocation (optionally).

Status: RESOLVED FIXED

(Tamarin Graveyard :: Tracing Virtual Machine, defect)

Description

[Andreas Gal :gal]

Attachment: avmplus.h used to embed nanojit in tracemonkey.

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.

Comment 1

[Andreas Gal :gal]

Attachment: Dummy GC data structure instantiations that go along with our custom avmplus.h

Comment 2

[Tommy Reilly]

+1 although I'd remove the "optional" bit, these ideas are all good in general.

Comment 3

[Jungwoo Ha]

Attachment: nanojit using explicit memory management in tamarin tracing

Comment 4

[Jungwoo Ha]

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. 

Comment 5

[Andreas Gal :gal]

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.

Comment 6

[Steven Johnson]

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

Comment 7

[Jungwoo Ha]

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?

Comment 8

[Andreas Gal :gal]

I think there was talk about making filters in the forward path (Interpreter) stack allocated. Ed?

Comment 9

[Edwin Smith]

yes, i would like to do that.  should we make it a prerequisite for this bug?

Comment 10

[Andreas Gal :gal]

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.

Comment 11

[Andreas Gal :gal]

Is anyone working on thus bug atm?

Comment 12

[Andreas Gal :gal]

Jungwoo, are you still working on this bug? 

Comment 13

[Jungwoo Ha]

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.

Comment 14

[Andreas Gal :gal]

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.

Comment 15

[Jungwoo Ha]

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.

Comment 16

[Andreas Gal :gal]

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. 

Comment 17

[Andreas Gal :gal]

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

Comment 18

[Andreas Gal :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.

Comment 19

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: explicit deallocation for fragments and friends

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

Comment 20

[Andreas Gal :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.

Comment 21

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 333294 [details] [diff] [review]
explicit deallocation for fragments and friends

Pushed.