Skip to content

Fixed a broken build with STRESS_DYNAMIC_HEAP_COUNT enabled - #105507

Merged
mrsharm merged 4 commits into
dotnet:mainfrom
mrsharm:fix_brokendynamic
Aug 8, 2024
Merged

Fixed a broken build with STRESS_DYNAMIC_HEAP_COUNT enabled#105507
mrsharm merged 4 commits into
dotnet:mainfrom
mrsharm:fix_brokendynamic

Conversation

@mrsharm

@mrsharm mrsharm commented Jul 25, 2024

Copy link
Copy Markdown
Member

Currently, enabling the STRESS_DYNAMIC_HEAP_COUNT stress mode doesn't build as a result of an undeclared num_gen2s_since_last_change when we fire the Size Adaptation Full GC Tuning Event:

        GCEventFireSizeAdaptationFullGCTuning_V1 (
            (uint16_t)dynamic_heap_count_data.new_n_heaps,
            (uint64_t)current_gc_index,
            (float)median_gen2_tcp,
            (uint32_t)num_gen2s_since_last_change, // error C2065: 'num_gen2s_since_last_change': undeclared identifier
            (uint32_t)(current_gc_index - gen2_samples[0].gc_index),
            (float)gen2_samples[0].gc_percent,
            (uint32_t)(current_gc_index - gen2_samples[1].gc_index),
            (float)gen2_samples[1].gc_percent,
            (uint32_t)(current_gc_index - gen2_samples[2].gc_index),
            (float)gen2_samples[2].gc_percent);

This fix ensures that with STRESS_DYNAMIC_HEAP_COUNT enabled, we can build successfully and emit the aforementioned event.

Thanks to @VincentBu for finding this issue.

@Maoni0

Maoni0 commented Jul 25, 2024

Copy link
Copy Markdown
Member

I noticed this too since I've been running with stress enabled. I fixed it differently by moving this line

#endif //STRESS_DYNAMIC_HEAP_COUNT

to be right before the last if block at the end of the method -

    if (new_n_heaps != n_heaps)
    {
        dprintf (6666, ("GC#%Id should change! %d->%d (%s)",
            VolatileLoadWithoutBarrier (&settings.gc_index), n_heaps, new_n_heaps, ((n_heaps < new_n_heaps) ? "INC" : "DEC")));
        dynamic_heap_count_data.heap_count_to_change_to = new_n_heaps;
        dynamic_heap_count_data.should_change_heap_count = true;
    }

@VincentBu VincentBu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We can build Core_Root with this fix when STRESS_DYNAMIC_HEAP_COUNT is enabled.

@cshung cshung left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@mrsharm
mrsharm merged commit 105bf4d into dotnet:main Aug 8, 2024
@github-actions github-actions Bot locked and limited conversation to collaborators Sep 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants