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.

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

Refactor segment size computation #71178

Merged
merged 2 commits into from
Jul 14, 2022

Conversation

cshung
Copy link
Member

@cshung cshung commented Jun 23, 2022

This change fixes the working set regression when running the JSON TechEmpower benchmark on Windows under a container. Here is a summary of the performance data:

Segments:
Max Working Set (MB) | 134

Regions before fix:
Max Working Set (MB) | 339

Regions after fix:
Max Working Set (MB) | 135

There is no significant difference between regions after fix and segment on other metrics we measured.

Cause of the regression:
The concept of SOH segment sizes does not exists on regions, but since the variable soh_segment_size is used in various performance tuning functions, we were hard coding that to 4G for workstation GC and 1G for server GC. It was mostly okay, but in an extreme situation like in this benchmark, we have only 512MB of memory but we are running on a machine with 28 cores => 28 heaps. The soh_segment_size is reduced significantly in the segment's case but we still use the same value under the region's case. This causes the performance tuning function to behave very differently and leads to regression.

Solution:
We keep the existing computation of the soh_segment_size logic under regions so that it will get back to what it was.

@ghost
Copy link

ghost commented Jun 23, 2022

Tagging subscribers to this area: @dotnet/gc
See info in area-owners.md if you want to be subscribed.

Issue Details

null

Author: cshung
Assignees: -
Labels:

area-GC-coreclr

Milestone: -

@cshung cshung changed the title Eliminate soh_segment_size from USE_REGIONS [WIP] Eliminate soh_segment_size from USE_REGIONS Jun 23, 2022
@cshung cshung force-pushed the public/eliminate-soh-segment-size branch from 28860e2 to d59a0a8 Compare June 23, 2022 18:24
@cshung cshung changed the title [WIP] Eliminate soh_segment_size from USE_REGIONS [WIP] Refactor segment size computation Jun 23, 2022
@cshung cshung changed the title [WIP] Refactor segment size computation Refactor segment size computation Jun 23, 2022
@Maoni0
Copy link
Member

Maoni0 commented Jun 25, 2022

you'd need to do a bit more work than just mechanically copying code.... I pushed a branch here: https://github.com/Maoni0/runtime/tree/soh_seg_size. note I just wrote this code. I haven't tried to compile it. this is just illustrating the idea of refactoring. the result caller looks like this

    size_t seg_size = 0;
    size_t large_seg_size = 0;
    size_t pin_seg_size = 0;

    if (gc_heap::heap_hard_limit)
    {
        if (!nhp_from_config)
        {
            nhp = gc_heap::adjust_heaps_hard_limit (nhp);
        }

        size_t seg_size_from_config = (size_t)GCConfig::GetSegmentSize();
        if (seg_size_from_config)
        {
            seg_size_from_config = adjust_segment_size_hard_limit_va (seg_size_from_config);
        }

        size_t limit_to_check = (gc_heap::heap_hard_limit_oh[soh] ? gc_heap::heap_hard_limit_oh[soh] : gc_heap::heap_hard_limit);
        soh_segment_size = max (adjust_segment_size_hard_limit (limit_to_check, nhp), seg_size_from_config);
    }
    else
    {
        soh_segment_size = get_valid_segment_size();
    }

#ifndef USE_REGIONS
    if (gc_heap::heap_hard_limit)
    {
        if (gc_heap::heap_hard_limit_oh[soh])
        {
            large_seg_size = max (adjust_segment_size_hard_limit (gc_heap::heap_hard_limit_oh[loh], nhp), seg_size_from_config);
            pin_seg_size = max (adjust_segment_size_hard_limit (gc_heap::heap_hard_limit_oh[poh], nhp), seg_size_from_config);
        }
        else
        {
            large_seg_size = gc_heap::use_large_pages_p ? soh_segment_size : soh_segment_size * 2;
            pin_seg_size = large_seg_size;
        }
        if (gc_heap::use_large_pages_p)
            gc_heap::min_segment_size = min_segment_size_hard_limit;
    }
    else
    {
        large_seg_size = get_valid_segment_size (TRUE);
        pin_seg_size = large_seg_size;
    }

Maoni0 and others added 2 commits July 7, 2022 13:22
@cshung cshung force-pushed the public/eliminate-soh-segment-size branch from bb098ef to 53e10e0 Compare July 7, 2022 20:22
@cshung cshung merged commit ea7aa9b into dotnet:main Jul 14, 2022
@cshung cshung deleted the public/eliminate-soh-segment-size branch July 14, 2022 19:59
@ghost ghost locked as resolved and limited conversation to collaborators Aug 14, 2022
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.

3 participants