631515 - Provide an API and implementation of a GC advice feature

Status: RESOLVED FIXED

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

Description

[Lars T Hansen]

Until we fix the incrementality bugs (bug 627025), and perhaps even later, it should be possible to instruct the VM programmatically about when it might be a good idea, and when it might be a bad idea, to finish garbage collection, or at least, when it is a good time to do a lot of work and when it is good to do a little or no work.  The programmer frequently knows this; the GC rarely does (though the interframe interval is a good time, see bug 535577; that bug might be able to make use of whatever we come up with here).

Some requirements:

- The API should be simple to use and hard to misuse; for example, if the API
  is an on/off type then it should be hard or impossible to forget to shut
  it off.

- The semantics should be such that there's little risk of perturbing the
  GC policy in major ways; for example, using it should not allow the
  effective load factor to double.

A strawman design:

  System.goodTimeToCollect(ms:int=1000, idle:boolean=false)

    During the next ms milliseconds we won't care much about GC pauses, so
    do as much as we can without starving the mutator.  Do a best-effort job
    at not overshooting the time budget.  If the end of the collection is
    "close" anyway, go ahead and finish it.

    If idle is true, the mutator can be starved.

    Cancels any badTimeToCollect that's currently in effect.

  System.badTimeToCollect(ms:int=1000, dontCollect:bool=false)

    During the next ms milliseconds we care a lot about GC pauses, so do only
    incremental work, even if this means using more memory (and risking an
    out-of-memory event).  When the timer expires the GC is at liberty to
    pause and finish the collection at any time, and if the next call to
    System.badTimeToCollect is the first opportunity to do so then that call
    may trigger the final phase of GC.

    If dontCollect is true don't even do incremental marking; the mutator 
    wants all the available time and promises not to allocate very much.

    Cancels any goodTimeToCollect that's currently in effect.

Comment 1

[Lars T Hansen]

Some more notes.

In order to satisfy the requirement of no-overshoot on goodTimeToCollect the GC may need to build up a model of how long typical GC pauses are for various kinds of operations.

ZCT reaping might be affected, certainly for badTimeToCollect with dontCollect=true.

Comment 2

[Lars T Hansen]

There should probably be a dead man's switch on badTimeToCollect, either in form of a limit on how much more memory we can use than the budget, or a maximum value for the timeout argument.  (Either would not be changeable by the user.)  It's possible that those limits would be platform-dependent -- different on desktop and mobile.  Both should be generous.

Comment 3

[Tommy Reilly]

From the player's perspective two points come to mind:

I would think they would want to call a C++ GC API to do this when the play timer is ahead of the game

Are we thinking about exposing the AS API in the player?   If so they would want to cap the time spent in collection, probably with a dynamic value, ie if video's playing they might want to effectively neuter goodTimeToCollect.

Comment 4

[Lars T Hansen]

(In reply to comment #3)
> From the player's perspective two points come to mind:
> 
> I would think they would want to call a C++ GC API to do this when the play
> timer is ahead of the game.

Probably.  The strawman uses AS3 just for convenience.

> Are we thinking about exposing the AS API in the player?

Absolutely.

> If so they would
> want to cap the time spent in collection, probably with a dynamic value, ie if
> video's playing they might want to effectively neuter goodTimeToCollect.

Can't do that.  The primary parameter for the garbage collector is the ratio of heap memory to live memory, not the time spent in GC.  At some point we have to stop and finish the collection.  Anyway discussions with Chris N have focused on pause time, not time spent in GC per se, as the main problem - I have this from other groups too.  It's the incrementality problem all over again and this is just a band-aid, though maybe a useful one (TBD).

(When we fix incrementality in MMgc the API proposed here may become significantly less relevant.)

Comment 5

[Tommy Reilly]

(In reply to comment #4)

> Can't do that.  

I meant cap the time we spend in goodTimeToCollect called from the application.  Ie we may want to allow the player to trump the flash applications attempt to influence the GC, ie set a max pause time or whatever.

Comment 6

[Lars T Hansen]

Patch queue for WIP at http://hg.mozilla.org/users/lhansen_adobe.com/redux-advice

Comment 7

[Lars T Hansen]

The design has been changed and simplified; the new design is on zerowing: flashruntime/Feature+Spec+GC+Advice

Comment 8

[Lars T Hansen]

Attachment: Implementation & tests

Preliminary.  Implementation of the new design, and a decent test case.

Comment 9

[Lars T Hansen]

Attachment: Implementation & tests

(Removed generated code.)

Comment 10

[Lars T Hansen]

Transfering ownership to Ruchi.

The C++ parts of the code need to be #ifdef'd, for easy removal.  The #ifdef name does not need to be exposed as an AVMFEATURE, in my opinion; it is only there until we've tested the API and found it to be acceptable.  After that the #ifdefs can be removed, leaving only the code.

There should be a comment with the C++ code that there is an AS3 API that must also be removed if the C++ code is removed.

Comment 11

[Lars T Hansen]

And by decision by PARB I think the API signature is now:

  function pauseForGCIfCollectionImminent(imminence:Number)

with details to be found on the spec page.  The comment block should be updated to hide data about the tie of imminence to the consumption of the allocation budget.

Comment 12

[Ruchi Lohani]

Attachment: Patch 2

Moved bool endOfCollection inside the #ifdef. Also are we looking to land this change first in FP mainline and then in TR as I don't think there will be a merge from TR to mainline this week ?

Comment 13

[Ruchi Lohani]

Just realized I also need to update the API name. Though the review can probably proceed without that as it does not affect the core functionality. I will get a new patch for the API name change.

Comment 14

[Ruchi Lohani]

Attachment: Patch 2

Question: do we want the Collect(double) method's parameter name to be changed from "allocationBudgetFractionUsed" to "imminence" and have comments above that explain : imminence is infact allocationBudgetFractionUsed? I am not in favor of changing it here as it will make the code hard to read(understand?).

Comment 15

[Tommy Reilly]

Comment on attachment 525519 [details] [diff] [review]
Patch 2

There needs to be a comment explaining what okToShrinkHeap is for.  Also I don't think its named very well, it certainly doesn't control any externally visible notion of the heap size (like whether we decommit or not).   To do that we would want to not call Decommit from Sweep.

Comment 16

[Lars T Hansen]

Following up to a couple of comments:

We should not rename the internal methods or parameters, only the public-facing API.

The okToShrinkHeap pertains to policy only; when the user asks for a preemptive GC he's really not interested in perturbing the heap size, as preemptive GC usually happens at a phase change and there's a risk the live set could be low, which would reduce the target heap size, which would cause a GC soonish.  That's not desirable.  There's a comment to that effect in the spec doc, I agree it would be good to have that in the code (or maybe the code should point to the spec do, except the code is open source and the spec doc is on zerowing).

There's a lot more we could do here, we could allow the user program to influence policy (eg, minimum heap size, load factors).  However, time is short.  So either this simple hack is effective for some class of programs, and we ship it, or we remove the API, without considering larger policy APIs at this point.

Comment 17

[Lars T Hansen]

Comment on attachment 525519 [details] [diff] [review]
Patch 2

The doc comment in shell_toplevel.as has to be fixed, it still talks about allocation budgets.

Comment 18

[Tommy Reilly]

okToShrinkHeap still seems like a sham to me since we still return pages to the GCHeap and we still call GCHeap::Decommit.   +1 if we go with okToShrinkPolicyHeapSize or allowBudgetReduction or something clearer.

Comment 19

[Lars T Hansen]

(In reply to comment #18)
> okToShrinkHeap still seems like a sham to me since we still return pages to the
> GCHeap and we still call GCHeap::Decommit.   +1 if we go with
> okToShrinkPolicyHeapSize or allowBudgetReduction or something clearer.

I don't care too much about the naming either way, but let's try to finish this patch today.  A comment goes a long way in clarifying what the parameter means.  There is such a comment on the GC API now.  Maybe that comment could be elaborated slightly.

Comment 20

[Tommy Reilly]

(In reply to comment #19)
> I don't care too much about the naming either way, but let's try to finish this
> patch today.  A comment goes a long way in clarifying what the parameter means.
>  There is such a comment on the GC API now.  Maybe that comment could be
> elaborated slightly.

I completely missed that comment, I was looking for a comment on Collect.   We should put a comment on the Collect call with the paramter too, even if its just "see the other Collect".   Based on that comment okToShrinkHeapTarget seems like the best name.

Comment 21

[Ruchi Lohani]

Attachment: Patch3

Fixed comments and changed the param name in Collect. Please push it for me once reviewed+. I need to get the changes in my sandbox to push to FP Mainline.

Comment 22

[Lars T Hansen]

I will push it to TR this afternoon if Tommy doesn't do it before then.

Comment 23

[Tamarin Bot]

changeset: 6183:70f637ae12cb
user:      Lars T Hansen <[e-mail]>
summary:   Fix 631515 - Provide an API and implementation of a GC advice feature (patch=rulohani, r=treilly, sr=lhansen, push=lhansen)

http://hg.mozilla.org/tamarin-redux/rev/70f637ae12cb

Comment 24

[Tamarin Bot]

changeset: 6185:3f106fb10339
user:      Brent Baker <[e-mail]>
summary:   Bug 631515: regenerate the shell_toplevel.* files since there was a new API added

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

Comment 25

[Ruchi Lohani]

Attachment: Remove throwing error

Comment 26

[Tamarin Bot]

changeset: 6293:ca2e23ce5c95
user:      Chris Peyer <[e-mail]>
summary:   Bug 631515: Remove throwing error and update test (p=rlohani, r=cpeyer)

http://hg.mozilla.org/tamarin-redux/rev/ca2e23ce5c95

Comment 27

[Brent Baker]

(In reply to comment #26)
> changeset: 6293:ca2e23ce5c95
> user:      Chris Peyer <[e-mail]>
> summary:   Bug 631515: Remove throwing error and update test (p=rlohani,
> r=cpeyer)
> 
> http://hg.mozilla.org/tamarin-redux/rev/ca2e23ce5c95

This patch has not landed in tr-serrano, reopening bug to make sure that this gets pushed into tamarin-redux-serrano (NOTE: change has already landed in Serrano_Anza, that is how I was able to notice that it was missing from tr-serrano)

Comment 28

[Ruchi Lohani]

The changes are already present in tamarin-redux-serrano. Marking Resolved-Fixed.