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

get rid of saved_allocated #77197

Closed
wants to merge 1 commit into from
Closed

Conversation

Maoni0
Copy link
Member

@Maoni0 Maoni0 commented Oct 19, 2022

I thought incorrectly when I made the change to use saved_allocated to calculate the frag ratio - this frag ratio actually should use the allocated that's adjusted by the plan phase, not the original allocated for the heap_segment. this can affect microbenchmarks dramatically when the last live object is way smaller than allocated, instead of doing a sweeping GC (which is the right choice) we ended up doing a compacting GC because we'd be calculating a very large frag ratio when we call decide_on_compacting.

@ghost
Copy link

ghost commented Oct 19, 2022

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

Issue Details

I thought incorrectly when I made the change to use saved_allocated to calculate the frag ratio - this frag ratio actually should use the allocated that's adjusted by the plan phase, not the original allocated for the heap_segment. this can affect microbenchmarks dramatically when the last live object is way smaller than allocated, instead of doing a sweeping GC (which is the right choice) we ended up doing a compacting GC because we'd be calculating a very large frag ratio when we call decide_on_compacting.

Author: Maoni0
Assignees: -
Labels:

area-GC-coreclr

Milestone: -

@mangod9
Copy link
Member

mangod9 commented Oct 19, 2022

Would this help reduce impact on String.Replace microbenchmark which was observed with regions?

@Maoni0
Copy link
Member Author

Maoni0 commented Oct 19, 2022

@mangod9 yes, I ran the simple string test and verified that it fixes the remaining regression. we'll be running more microbenchmarks.

@mangod9
Copy link
Member

mangod9 commented Mar 20, 2023

@Maoni0, is this PR still relevant?

@Maoni0
Copy link
Member Author

Maoni0 commented Mar 21, 2023

it is. I just didn't have time to take care of it. will do so soon.

@stephentoub
Copy link
Member

@Maoni0, looking at old PRs... should this be closed?

@Maoni0
Copy link
Member Author

Maoni0 commented Jul 22, 2024

I'm closing this for now. I've added this to my list of items to keep track of.

@Maoni0 Maoni0 closed this Jul 22, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Aug 22, 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.

3 participants