Skip to content

get rid of saved_allocated#77197

Closed
Maoni0 wants to merge 1 commit intodotnet:mainfrom
Maoni0:saved_allocated
Closed

get rid of saved_allocated#77197
Maoni0 wants to merge 1 commit intodotnet:mainfrom
Maoni0:saved_allocated

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 ghost added the area-GC-coreclr label Oct 19, 2022
@ghost ghost assigned Maoni0 Oct 19, 2022
@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.

This was referenced Oct 19, 2022
Closed
Closed
Closed
@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

stephentoub commented Jul 22, 2024

@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.

Reviewers

No reviews

Assignees

@Maoni0 Maoni0

Labels

area-GC-coreclr

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Maoni0 @mangod9 @stephentoub