I've heard several reports from people that most of their incremental GCs are getting reset, which makes incremental GC pretty pointless. This patch tries to reduce the number of resets that happen. It also reports the reason for a reset, so that we can track these problems better in the future. Currently, when we enter a GC, the only alternative to doing an incremental slice is to reset the current incremental GC and do a non-incremental GC right away. This is overly conservative. With the patch, we now have several choices: 1. Do an incremental slice normally. 2. Reset the incremental GC and start a new incremental GC. The current slice will be the first one of the new GC. 3. Finish the current incremental GC non-incrementally. 4. Reset the current incremental GC and do a non-incremental GC. (This is the same as before.) In my testing, the most common reason for resets was that we would start the incremental GC as a compartmental GC. In a later slice, we would be asked to do a full GC. Without the patch, we would have to reset and proceed non-incrementally. With the patch, we can take option 2 above and continue to use incremental GC (although we still have to throw away work from the compartmental GC). We now report resets on a per-slice basis, since they can happen any number of times in a given GC. If we ever switch to non-incremental GC, that is reported once, since it can only happen once per GC. Also, this patch changes the meaning of a telemetry counter slightly (GC_RESET). We've only had this counter in for a week, and the meaning doesn't change much, so I think it's okay.

Comment on attachment 601116 [details] [diff] [review] patch What the patch is doing seems reasonable, but I do not like passing C strings as out params especially in most cases that out param result is not used. Either make the reason out param optional or reverse the meaning of IsIncrementalGCSafe to IsUnsafeIncrementalGC and return a C string, not a bool from it, when it is unsafe and NULL when it is safe. This at least will be consistent with the usage of NULL char* pointer in other parts of the patch. A fancier schema is to to add a class like IncrementalStatus that wraps const char * with methods like ok() and reason() and return the class from IsIncrementalGCSafe renamed to GetIncrementalGCStatus(), but that could be too much code to add. string literals around to communicate meaningful information especially when that information is often discarded like in the couple of cases when IsIncrementalGCSafe is called. Either Either add a enum that IsIncrementalGCSafe returns or ResetIncrementalGC takes or at least

I made a special class for the result of IsIncrementalGCSafe. https://hg.mozilla.org/integration/mozilla-inbound/rev/1e24d9b1f0f4