490378 - GCCallbacks interface needs a callback entry suitable for cache flushing

Status: VERIFIED FIXED

(Tamarin Graveyard :: Garbage Collection (mmGC), defect, P2)

Description

[Edwin Smith]

rough criteria
 - memory is low
 - tell jit cache (or any other big cache) to shrink

Lars suggested adding a callback at the top of StartIncrementalMark

Comment 1

[Lars T Hansen]

In general, we need to establish invariants for the callbacks: what they can and cannot do, and what state the system may be in.  This is probably a significant design exercise.

I think a callback at StartIncrementalMark is generally useful, and should probably be unconditional.  I don't know whether it's useful for that callback to be conditional on or pass information about the memory state - maybe it's useful to know whether the gc was triggered by memory allocation or was an explicit gc.  In the latter case the caches probably don't want to clear themselves.

My suggestion to Ed about adding a callback there was mostly intended to let him get on with testing without worrying about MMgc being in a critical state.

Comment 2

[Edwin Smith]

I've added the callback there, and it basically works, with one caveat.  If any allocations occur on the callback, StartIncrementalMark will get called again (since the conditions for calling it in the first place haven't changed).  It's easy to ignore the second call into the callback, but it doesn't help; the inner StartIncrementalMark eventually progresses, and when the stack unwinds, the outer one asserts (!marking).

So I have guards in the callback to assert on re-entry, and have refactored the code so freeing code blocks doesn't trigger more allocations.

the mmgc patch is trivial but i'll hold off for further testing before trying to land it. (unless it's a keeper?)

Comment 3

[Edwin Smith]

Attachment: adds premark() callback in StartIncrementalMark

Also fixes comment on GC::Alloc

I'm using this callback to prod the code manager to flush, but its
not necessarily the best place to do the prodding.  Using a hook this
frequent in production would require the callee (CodeMgr) to add more
logic to flush less often; a more appropriate hook would be better.

R+ if we should land this anyway, at the risk of it becoming dead code later.
R- with suggestions if there's a better hook to use

Comment 4

[Lars T Hansen]

Comment on attachment 376233 [details] [diff] [review]
adds premark() callback in StartIncrementalMark

As you say, it may go away later, but this is fine for now.

Comment 5

[Edwin Smith]

Comment on attachment 376233 [details] [diff] [review]
adds premark() callback in StartIncrementalMark

http://hg.mozilla.org/tamarin-redux/rev/3d948a7df208

Comment 6

[Tommy Reilly]

This need is met by the OOMCallback API