Closed Bug 963232 Opened 6 years ago Closed 6 years ago

Beef up the comments in GCAPI

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: terrence, Assigned: terrence)

Details

Attachments

(1 file, 1 obsolete file)

I did not realize how bad the situation was until I dove in to try and help Yves figure out why his incremental collections are not doing anything. It turns out nothing here works quite the way I thought it was supposed to work from looking solely from the inside. The comments I've added basically comprise my notes while digging. I think these are a definite improvement, but further ideas are, of course, welcome.

Bill, I'm flagging you for review so you can double-check my understanding of how the incremental API is supposed to work. I think I understand it now, but there is quite a bit of subtlety there and I don't want to accidentally mislead anyone into using a suboptimal strategy.
Attachment #8364555 - Flags: review?(wmccloskey)
Comment on attachment 8364555 [details] [diff] [review]
document_gcapi-v0.diff

Review of attachment 8364555 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for writing this!

::: js/public/GCAPI.h
@@ +125,5 @@
> + * When performing an incremental GC, the zones that were selected for the
> + * previous incremental slice must be selected in subsequent slices as well.
> + * This function selects those slices automatically. This is only necessary
> + * when triggering GC slices manually. Most users will want to use
> + * NotifyDidPaint, which calls this automatically.

I don't think we should recommend that anyone call NotifyDidPaint. It's unfortunate that we even expose it. Ideally that code would go in XPConnect or someplace like that.

@@ +132,5 @@
>  PrepareForIncrementalGC(JSRuntime *rt);
>  
> +/*
> + * Returns true if any zone in the system has been scheduled for GC with one of
> + * the functions above.

...or by the JS engine.

@@ +147,5 @@
>  
>  /*
> + * Full GC:
> + *
> + * The following functions perform a non-incremental GC.

Better not to use the term "full" since it's ambiguous. Just say non-incremental.

@@ +152,4 @@
>   */
>  
> +/*
> + * Performs a full, non-incremental collection. Some objects that are

Especially important not to say full here, since it can perform a zone GC depending on which Prepare functions were called.

@@ +153,5 @@
>  
> +/*
> + * Performs a full, non-incremental collection. Some objects that are
> + * unreachable from the program may still be alive afterwards because of
> + * caching in the JSRuntime.

Shrinking GCs aren't exactly related to caching. I guess you could call the chunks we store a way a "cache", but not exactly.

@@ +159,5 @@
>  extern JS_FRIEND_API(void)
>  GCForReason(JSRuntime *rt, gcreason::Reason reason);
>  
> +/*
> + * Perform a full, non-incremental collection after clearing caches and other

Same comment about full here. Also, I filed bug 963243 about this function. It looks like we don't use it properly.

@@ +188,5 @@
> + *
> + * Note: Typically, an embedding will not want to call IncrementalGC manually.
> + *       Instead it should rely on the GC triggers to call it on an as needed
> + *       basis and simply indicate good times for GC work to happen by calling
> + *       NotifyDidPaint at regular intervals.

Please take out this paragraph. The engine has no built-in functionality to start incremental GCs. It's only done by API functions. JS_MaybeGC will start an incremental GC if memory is low. We shouldn't mention NotifyDidPaint. The main entry point to get an incremental GC is IncrementalGC--that should be emphasized.

@@ +209,5 @@
>  
> +/*
> + * If IsIncrementalGCInProgress(rt), this call finishes the ongoing collection
> + * by performing an arbitrarily long slice. If !IsIncrementalGCInProgress(rt),
> + * this is equivalent to GCForReason.

Perhaps add that IsIncrementalGCInProgress(rt) is always false when this returns.

@@ +245,5 @@
>  typedef void
>  (* GCSliceCallback)(JSRuntime *rt, GCProgress progress, const GCDescription &desc);
>  
> +/*
> + * The GC slice callback is called multiple times per slice with |progress| set

Maybe just say it's called at the beginning and end of every slice.

@@ +255,5 @@
>  
>  /*
> + * Causes the currently running GC to be reset and restart from the beginning.
> + * This is only effectful when called from the GCSliceCallback when
> + * GC_SLICE_END or GC_CYCLE_END.

This is a horrible function that we should not say anything about. Just say it's internal to Firefox.

(nsJSEnvironment has a function called PokeGC that is completely different from this.)

@@ +263,5 @@
> +
> +/*
> + * Signals to the GC that this is a good place to do an incremental slice. For
> + * interactive applications, this will typically be the start or end of each
> + * frame of animation.

As I said, please just say that this function is internal to Firefox.

@@ +320,5 @@
>  extern JS_FRIEND_API(void)
>  IncrementalObjectBarrier(JSObject *obj);
>  
> +/*
> + * Returns true if the most recent GC run incrementally.

run -> ran
Attachment #8364555 - Flags: review?(wmccloskey) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/8eb896135c62

Thanks for the clarification, Bill!

I've updated the comments as requested, but I'm not so clear anymore on how embedders are supposed to trigger GC. In particular, are they required to pump slices manually if an internal JS_MaybeGC starts an incremental GC? Also, I'd think that "GC at the start of a frame" is going to be a pretty common need for people embedding js in an interactive context, so I'm not so sure why NotifyDidPaint is a bad API to have available.
Thanks for working on this and helping to make it easier for SpiderMonkey embedders!

I've tested MaybeGC and looked at the code.
I'm quite sure it can't be used to run an incremental GC. The condition rt->gcIsNeeded will never be true there as far as I can tell and below it checks for rt->gcIncrementalState == NO_INCREMENTAL, so it won't call that either in incremental mode.

I got it working quite well for 0 A.D. now. If the heap size grows more than 20 MB after a GC, it starts running incremental slices of 10ms each turn. I had to implement that logic in our code.
I've attached the relevant parts if you're interested how it looks at the moment.

Some more detailed comments about the JSGCParamKey structure and the different settings would also be helpful.
I figured out the most important ones with some testing but it took quite a while. Also adding the units for the values would be a good idea. I assumed JSGC_ALLOCATION_THRESHOLD is in bytes because other settings are in bytes too. Later I wondered why it doesn't work as expected and figured out it's in Megabyte.

I'm also having a problem with a zone that is left over from a destroyed JSContext. It causes incremental GC to fail.
At the moment I'm using a workaround, but I'll try to figure out what the root cause is.
https://hg.mozilla.org/mozilla-central/rev/8eb896135c62
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Comment on attachment 8365598 [details]
The current WIP implementation of incremental GC for 0 A.D. (only the relevant parts of the ScriptInterface.cpp file)

There are still several problems with this version.
If anyone is interested in the current version, please ping me on IRC or write a mail.
Attachment #8365598 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.