[draft] [GC] Don't promote gen0 when it is mostly objects moved to the finalization queue #109085
Conversation
| bool do_promote = ((threshold > older_gen_size) || (promoted > threshold)); | ||
|
|
||
| dprintf (6666, ("h%d promotion threshold: %zd, promoted bytes: %zd size n+1: %zd -> %s", | ||
| i, threshold, promoted, older_gen_size, | ||
| (((threshold > (older_gen_size)) || (promoted > threshold)) ? "promote" : "don't promote"))); | ||
| (do_promote ? "promote" : "don't promote"))); | ||
|
|
||
| if ((threshold > (older_gen_size)) || (promoted > threshold)) | ||
| if (do_promote) |
There was a problem hiding this comment.
simple factoring from when I was initially changing this code
| int finalization_percent = (total_promoted_bytes > 0) | ||
| ? static_cast<int>((100 * finalization_promoted_bytes) / total_promoted_bytes) : 0; | ||
| dprintf (6666, ("finalization promotion: %d%% (%zd / %zd)", finalization_percent, finalization_promoted_bytes, total_promoted_bytes)); | ||
| high_finalization_percentage = finalization_percent >= 90; |
There was a problem hiding this comment.
90? Microbenchmark is showing 95-98, so this works for it, but 90 is awfully high.
There was a problem hiding this comment.
6666 is probably not the right dprintf level here
| @@ -45045,6 +45050,11 @@ BOOL gc_heap::decide_on_compacting (int condemned_gen_number, | |||
| should_compact = TRUE; | |||
| } | |||
|
|
|||
| if (high_finalization_percentage) | |||
| { | |||
| should_compact = TRUE; | |||
There was a problem hiding this comment.
currently not-compacting causes us to enable promotion below
| dprintf(1, ("Finalizer counts @%5s; 0: %6zu; 1: %6zu, 2: %6zu; CF: %6zu; F: %6zu; Free: %6zu", | ||
| label, genCounts[0], genCounts[1], genCounts[2], | ||
| SegQueueCount(CriticalFinalizerListSeg), SegQueueCount(FinalizerListSeg), SegQueueCount(FreeListSeg))); | ||
| } |
There was a problem hiding this comment.
Existing event only records how much is added to the finalization list. This lets us watch the size over time. I think this is probably worth a new event as well, but I haven't decided what it should be. (perhaps as minimal as just the "start" ones but only if something is non-zero?)
There was a problem hiding this comment.
probably shouldn't be dprintf(1)
There was a problem hiding this comment.
add event for beginning of GC - add to HeapStats?
| FIRE_DPRINTF_EVENT(GCHeapStats_V2, 1, | ||
| "0: %zu(%zu); 1: %zu(%zu); 2: %zu(%zu); 3: %zu(%zu); 4: %zu(%zu); final: %zu bytes(%zu); pin: %zu; sync: %zu; gchandles: %zu", |
There was a problem hiding this comment.
an idea for removing the code duplication here -
This was useful to me because I was using other dprintfs (not an event log) to analyze this PR, but the checked-in dprintf was missing information that the event had.
The downside is that one can't update the dprintf because it has to match the event. Notice that I had to drop the gc_index from the output, so one now has to go find the *GC* line.
There was a problem hiding this comment.
need to undo change to dprintf level 1
|
Tagging subscribers to this area: @dotnet/gc |
|
Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it. |
There are several separate questions and cleanup ideas - see GH comments for the code