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

Added the fix to make use of the heap hard limit instead of the region range while using large pages. #68629

Closed
wants to merge 1 commit into from

Conversation

mrsharm
Copy link
Member

@mrsharm mrsharm commented Apr 27, 2022

Summary

If large pages are enabled, we set the reserve_size of regions to be equivalent to that of the heap hard limit rather than the default range.

@ghost
Copy link

ghost commented Apr 27, 2022

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

Issue Details

Summary

Addresses #54616 for which, if large pages are enabled, we are setting the reserve_size of regions to be equivalent to that of the heap hard limit rather than the default range.

Author: mrsharm
Assignees: mrsharm
Labels:

area-GC-coreclr

Milestone: -

Copy link
Member

@cshung cshung left a comment

Choose a reason for hiding this comment

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

LGTM

@Maoni0
Copy link
Member

Maoni0 commented May 3, 2022

what kind of perf testing was done on this? I believe using the hardlimit is way too strict here and you will end up not being able to realistically allocate the hardlimit amount of memory. in segment cases we actually commit 2x the limit the user specifies for SOH and LOH because we cannot predict if objects will be large or small. with regions we certainly are working toward getting rid of this distinction but it would require some thoughts on what our policy should be.

@mrsharm
Copy link
Member Author

mrsharm commented May 4, 2022

what kind of perf testing was done on this? I believe using the hardlimit is way too strict here and you will end up not being able to realistically allocate the hardlimit amount of memory. in segment cases we actually commit 2x the limit the user specifies for SOH and LOH because we cannot predict if objects will be large or small. with regions we certainly are working toward getting rid of this distinction but it would require some thoughts on what our policy should be.

There was no perf testing / tuning done for this PR. I can work on this after discussing the policy here.

@trylek
Copy link
Member

trylek commented Jun 20, 2022

This is starting to show up on our stale PR radar - what are the next steps to proceed here?

@mrsharm
Copy link
Member Author

mrsharm commented Jun 21, 2022

@trylek - closing for now. Will re-open when we have a more concrete solution.

@mrsharm mrsharm closed this Jun 21, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jul 22, 2022
@mrsharm mrsharm reopened this Aug 4, 2022
@mrsharm
Copy link
Member Author

mrsharm commented Aug 4, 2022

Without my change, we are now committing 2 x the heap hard limit based on the region_range variable's value and this is the behavior we expect as region_range is computed in the following way if heap_hard_limit is set:

 gc_heap::regions_range = (size_t)GCConfig::GetGCRegionRange();
    if (gc_heap::regions_range == 0)
    {
        if (gc_heap::heap_hard_limit)
        {
            gc_heap::regions_range = 2 * gc_heap::heap_hard_limit;
        }
        else
        {
            gc_heap::regions_range = max(((size_t)256 * 1024 * 1024 * 1024), (size_t)(2 * gc_heap::total_physical_mem));
        }
        gc_heap::regions_range = align_on_page(gc_heap::regions_range);
    }

@mrsharm mrsharm closed this Aug 4, 2022
@mrsharm mrsharm deleted the largepage_regions branch August 4, 2022 21:20
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.

None yet

4 participants