Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[draft] [GC] Don't promote gen0 when it is mostly objects moved to the finalization queue #109085

Closed
wants to merge 6 commits into from
Closed

[draft] [GC] Don't promote gen0 when it is mostly objects moved to the finalization queue #109085

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
88a4e9b
factor
markples Oct 18, 2024
f862654
CFinalize::LogCounts
markples Oct 18, 2024
0f97e17
FIRE_DPRINTF_EVENT
markples Oct 18, 2024
e802e9c
decode_on_promotion_surv is ISOLATED
markples Oct 18, 2024
115ab2b
don't promote with high finalization percentage
markples Oct 19, 2024
d0e5858
remove now-redundant dprintf
markples Oct 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 34 additions & 5 deletions src/coreclr/gc/gc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2966,6 +2966,7 @@ dynamic_data gc_heap::dynamic_data_table [total_generation_count];
gc_history_per_heap gc_heap::gc_data_per_heap;
size_t gc_heap::total_promoted_bytes = 0;
size_t gc_heap::finalization_promoted_bytes = 0;
bool gc_heap::high_finalization_percentage = false;
size_t gc_heap::maxgen_pinned_compact_before_advance = 0;

uint8_t* gc_heap::alloc_allocated = 0;
Expand Down Expand Up @@ -29535,12 +29536,13 @@ BOOL gc_heap::decide_on_promotion_surv (size_t threshold)
size_t older_gen_size = dd_current_size (dd) + (dd_desired_allocation (dd) - dd_new_allocation (dd));

size_t promoted = hp->total_promoted_bytes;
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)
Comment on lines +29539 to +29545
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

simple factoring from when I was initially changing this code

{
return TRUE;
}
Expand Down Expand Up @@ -30230,6 +30232,7 @@ void gc_heap::mark_phase (int condemned_gen_number)
#endif //FEATURE_PREMORTEM_FINALIZATION

total_promoted_bytes = get_promoted_bytes();
finalization_promoted_bytes = total_promoted_bytes - promoted_bytes_live;

#ifdef MULTIPLE_HEAPS
static VOLATILE(int32_t) syncblock_scan_p;
Expand Down Expand Up @@ -30280,7 +30283,11 @@ void gc_heap::mark_phase (int condemned_gen_number)
#endif //FEATURE_EVENT_TRACE

//decide on promotion
if (!settings.promotion)
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;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

90? Microbenchmark is showing 95-98, so this works for it, but 90 is awfully high.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

6666 is probably not the right dprintf level here

if (!settings.promotion && !high_finalization_percentage)
{
size_t m = 0;
for (int n = 0; n <= condemned_gen_number;n++)
Expand Down Expand Up @@ -30360,8 +30367,6 @@ void gc_heap::mark_phase (int condemned_gen_number)
merge_mark_lists (total_mark_list_size);
#endif //MULTIPLE_HEAPS && !USE_REGIONS

finalization_promoted_bytes = total_promoted_bytes - promoted_bytes_live;

mark_queue.verify_empty();

dprintf(2,("---- End of mark phase ----"));
Expand Down Expand Up @@ -45045,6 +45050,11 @@ BOOL gc_heap::decide_on_compacting (int condemned_gen_number,
should_compact = TRUE;
}

if (high_finalization_percentage)
{
should_compact = TRUE;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

currently not-compacting causes us to enable promotion below

}

dprintf (2, ("Fragmentation: %zu Fragmentation burden %d%%",
fragmentation, (int) (100*fragmentation_burden)));

Expand Down Expand Up @@ -52176,6 +52186,19 @@ void CFinalize::WalkFReachableObjects (fq_walk_fn fn)
}
}

void CFinalize::LogCounts(const char* label)
{
size_t genCounts[max_generation + 1] = {};
for (int gen = 0; gen <= max_generation; ++gen)
{
unsigned int seg = gen_segment(gen);
genCounts[gen] = SegQueueCount(seg);
}
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)));
}
Copy link
Member Author

@markples markples Oct 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably shouldn't be dprintf(1)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#ifdef

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add event for beginning of GC - add to HeapStats?


BOOL
CFinalize::ScanForFinalization (promote_func* pfn, int gen, gc_heap* hp)
{
Expand All @@ -52189,6 +52212,8 @@ CFinalize::ScanForFinalization (promote_func* pfn, int gen, gc_heap* hp)
sc.thread_count = 1;
#endif //MULTIPLE_HEAPS

LogCounts("start");

BOOL finalizedFound = FALSE;

//start with gen and explore all the younger generations.
Expand Down Expand Up @@ -52279,6 +52304,8 @@ CFinalize::ScanForFinalization (promote_func* pfn, int gen, gc_heap* hp)
}
}

LogCounts("scan");

return finalizedFound;
}

Expand Down Expand Up @@ -52358,6 +52385,8 @@ CFinalize::UpdatePromotedGenerations (int gen, BOOL gen_0_empty_p)
}
}
}

LogCounts("promo");
}

BOOL
Expand Down
13 changes: 10 additions & 3 deletions src/coreclr/gc/gcee.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,15 +166,22 @@ void GCHeap::UpdatePostGCCounters()
FIRE_EVENT(GCEnd_V1, static_cast<uint32_t>(pSettings->gc_index), condemned_gen);

#ifdef SIMPLE_DPRINTF
dprintf (2, ("GC#%zu: 0: %zu(%zu); 1: %zu(%zu); 2: %zu(%zu); 3: %zu(%zu)",
dprintf (1, ("GC#%zu: 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",
(size_t)pSettings->gc_index,
g_GenerationSizes[0], g_GenerationPromotedSizes[0],
g_GenerationSizes[1], g_GenerationPromotedSizes[1],
g_GenerationSizes[2], g_GenerationPromotedSizes[2],
g_GenerationSizes[3], g_GenerationPromotedSizes[3]));
g_GenerationSizes[3], g_GenerationPromotedSizes[3],
g_GenerationSizes[4], g_GenerationPromotedSizes[3],
promoted_finalization_mem,
GetFinalizablePromotedCount(),
static_cast<uint32_t>(total_num_pinned_objects),
total_num_sync_blocks,
static_cast<uint32_t>(total_num_gc_handles)));
#endif //SIMPLE_DPRINTF

FIRE_EVENT(GCHeapStats_V2,
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",
Comment on lines +168 to +169
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to undo change to dprintf level 1

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cut

g_GenerationSizes[0], g_GenerationPromotedSizes[0],
g_GenerationSizes[1], g_GenerationPromotedSizes[1],
g_GenerationSizes[2], g_GenerationPromotedSizes[2],
Expand Down
5 changes: 5 additions & 0 deletions src/coreclr/gc/gceventstatus.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,4 +289,9 @@ void FireDynamicEvent(const char* name, EventArgument... arguments)
#define FIRE_EVENT(name, ...) 0
#endif // FEATURE_EVENT_TRACE

#define FIRE_DPRINTF_EVENT(name, level, format, ...) do { \
FIRE_EVENT(name, __VA_ARGS__); \
dprintf(level, (format, __VA_ARGS__)); \
} while(0)

#endif // __GCEVENTSTATUS_H__
10 changes: 8 additions & 2 deletions src/coreclr/gc/gcpriv.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2818,7 +2818,7 @@ class gc_heap

PER_HEAP_ISOLATED_METHOD int get_num_heaps();

PER_HEAP_METHOD BOOL decide_on_promotion_surv (size_t threshold);
PER_HEAP_ISOLATED_METHOD BOOL decide_on_promotion_surv (size_t threshold);

PER_HEAP_METHOD void mark_phase (int condemned_gen_number);

Expand Down Expand Up @@ -3474,6 +3474,7 @@ class gc_heap

PER_HEAP_FIELD_SINGLE_GC size_t total_promoted_bytes;
PER_HEAP_FIELD_SINGLE_GC size_t finalization_promoted_bytes;
PER_HEAP_FIELD_SINGLE_GC bool high_finalization_percentage;

PER_HEAP_FIELD_SINGLE_GC size_t mark_stack_tos;
PER_HEAP_FIELD_SINGLE_GC size_t mark_stack_bos;
Expand Down Expand Up @@ -5660,7 +5661,10 @@ class CFinalize
{
return (Seg == MaxSeg ? m_EndArray : m_FillPointers[Seg]);
}

inline size_t SegQueueCount (unsigned int Seg)
{
return SegQueueLimit(Seg) - SegQueue(Seg);
}
size_t UsedCount ()
{
return (SegQueue(FreeListSeg) - m_Array) + (m_EndArray - SegQueueLimit(FreeListSeg));
Expand All @@ -5672,6 +5676,8 @@ class CFinalize
return (SegQueueLimit(i) == SegQueue (i));
}

void LogCounts(const char* label);

public:
~CFinalize();
bool Initialize();
Expand Down
Loading