624590 - TM: properly handle OOMs from Nanojit

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

TM's handling of OOMs, especially in Nanojit, has various problems.  Bug 623428 will fix several of them for Firefox 4.0, but we need to improve things further after that.

The first step is to make the infallible allocators (both code and data) used for Nanojit better.  After bug 623428 we'll have a reserve that (a) wastes space, and (b) may run out (leading to an abort) if OOM checks aren't frequent enough.  We should instead just setjmp/longjmp to a safe place.  Since the tracer is entirely optional, this shouldn't be a problem, it's just a matter of getting the unwinding right and not failing to deallocate any code.  In theory NJ and TM are set up to do this... bug 623428 discusses this in some detail.  Bug 559707 covers the problems with the code allocator, this bug subsumes it.
AFAICT, if anything is done incorrectly it'll just lead to memory leaks;  manual OOM injection combined with Valgrind should do a good job of hunting down any such cases, hopefully.

There are also a couple of places -- the FrameInfo and TraceRecorder constructors -- where the tracer currently aborts on OOM, presumably because you can't return a failure result easily from a constructor.  This should be fixed.

As part of this, the use of fallible allocation for Nanojit's CseFilter introduced by bug 623428 should be removed -- those allocations are pretty small (eg. 32KB is a really big one) and so they don't really need to be fallible, it was just a sop to TM's use of a reserve to fake infallible alloc.
The fallible allocator might be worth removing altogether from NJ as NJ currently doesn't have much in the way of big (eg. multi-MB) allocations that are recoverable from if they fail.

Also, I currently get various "out of memory" error messages if I inject deliberate allocation failures in the VMAllocators.  There's no need for these to happen, the tracer should abort completely in case of OOM and normal non-traced execution should continue.

Comment 2

[Nicholas Nethercote [inactive]]

Also, TM uses dataAlloc for Assembler() when it seems like it could use an allocator with a shorter lifetime.

Also, dataAlloc/traceAlloc/tempAlloc are terrible names, I can never remember which is which.

Comment 3

[Nicholas Nethercote [inactive]]

Also worth noting:  Nanojit uses arena/region-style allocation exclusively.  This is great for most things, because compilers have nice phase structure, but there's at least one case where it's bad -- in CseFilter we often have to resize the hashtables.  Every time we do this, the space taken up by the old table is just wasted until the relevant arena/region is freed.  Having an additional malloc/free style allocator for Nanojit would avoid this problem.

Comment 4

[Edwin Smith]

At the time I converted most code to use arenas, we talked about that wasted space and agreed it wasn't very important to worry about it.  What I was prepared to do instead, and we still could do, is develop a different kind of hashtable that didn't require contiguous space -- possibly just using buckets.

then again, since arenas already require a malloc/free style api anyway for allocating chunks, it seems fine to factor that out and make it generally available to nanojit code, and use it in CSEFilter.

Comment 5

[Nicholas Nethercote [inactive]]

(In reply to comment #2)
> Also, TM uses dataAlloc for Assembler() when it seems like it could use an
> allocator with a shorter lifetime.

Turn out this isn't quite true, because the Assembler object itself has the same lifetime as the TraceMonitor, because it is kept around and reused for assembling multiple LIR fragments.  So things like the _branchStateMap() and other pieces that are cleared in arReset() need to be allocated with dataAlloc.

However, there are a couple of pieces allocated with that same allocator that should use a more temporary allocator, AFAICT -- 'pending_lives' in gen() and 'st' in LabelStateMap::add() should both be allocated with TM's 'tempAlloc' allocator.

(Note also that the 'alloc' argument in Assembler::compile() shadows the 'alloc' member;  this is appropriate, because we want to use TM's 'tempAlloc' in compile(), but it's rather confusing.)

Comment 6

[Nicholas Nethercote [inactive]]

Also, the alloc.alloc() calls in emitJumpTable() should really be dataAlloc.alloc().  We get away with this for TM because we pass in (TM's) dataAlloc, but it's conceivable that something with a shorter life-time could be passed in.  (TR doesn't use LIR_xtbl and so emitJumpTable() isn't used).

Comment 7

[Nicholas Nethercote [inactive]]

TM's days are numbered:  WONTFIX.