508610 - Need an GCHeap config variable where a client can set a soft heap limit

Status: VERIFIED FIXED

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

Comment 1

[Tommy Reilly]

The idea is when we hit this point we go from kMemNormal to kMemReserve and when we drop below this limit we can go back to kMemNormal.  

Questions: to we send an additional kMemReserve message when memory runs out?  Idea is we want to do another GC at that point where we can

This soft limit should be runtime variable that the client can change, idea is that if the OS signal's memory is low before we get to the soft limit the program can lower the soft limit to the current heap size.

Comment 2

[Tommy Reilly]

Attachment: Adds soft limit and centralizes kMemNormal transition checking

Comment 3

[Lars T Hansen]

Comment on attachment 396284 [details] [diff] [review]
Adds soft limit and centralizes kMemNormal transition checking

The second test in CheckForStatusReturnToNormal appears to be wrong.  Let p be the peak number of blocks and s be the current number of blocks.  The test is currently
  10p < 9s
that is
  10/9 p < s
which is to say, current must be greater than peak which is absurd.  Switching the 10 and the 9 around gives you 
  9/10 p < s
which sounds better, but that's still the wrong test: you want the opposite of that, 
  9/10 p > s.

All that said, this check sounds ad-hoc and I'm not sure what the motivation is.  Do you have data or anything else to back it up with?

As a style recommendation, I see there are a couple of places where combinations of flags are being tested - it might be useful to encapsulate these as named tests (inline functions) and just reference them, eg, inLowMemoryStatus() would test for kMemReserve || kMemSoftLimit.

R+ but with the proviso that the buggy 10% computation is fixed or removed, and backed up with more data/evidence/reasoning if not removed.

Comment 4

[Tommy Reilly]

Cool, yeah in the latest version of this in the player that test was fixed by the people who are relying on the soft limit stuff.   +1 on the meaningful inlines too

Comment 5

[Tommy Reilly]

changeset:   2439:0d25988a326d