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

Fixed a WKS specific issue by not clobbering the heap_segment_saved_allocated #72923

Merged
merged 2 commits into from
Aug 1, 2022

Conversation

mrsharm
Copy link
Member

@mrsharm mrsharm commented Jul 27, 2022

For WKS, we weren't using the correct heap_segment_saved_allocated value as it was overwritten in the plan phase and as a result, we didn't compact causing us to fill up the decommit list and continually call virtual_commit and virtual_decommit.

The fix, specifically for WKS, involves not overwriting the value of heap_segment_saved_allocated if it has already been set.

This PR is an effort to fix microbenchmarks that regressed after regions were introduced such as: #67909.

@ghost
Copy link

ghost commented Jul 27, 2022

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

Issue Details

For WKS, we weren't using the correct heap_segment_saved_allocated value as it was overwritten in the plan phase and as a result, we didn't compact causing us to fill up the decommit list and continually call virtual_commit and virtual_decommit.

The fix, specifically for WKS, involves not overwriting the value of heap_segment_saved_allocated if it has already been set.

This PR is an effort to fix microbenchmarks that regressed after regions were introduced such as: #67909.

Author: mrsharm
Assignees: -
Labels:

area-GC-coreclr

Milestone: -

@mrsharm mrsharm marked this pull request as ready for review July 27, 2022 08:46
@mrsharm mrsharm changed the title Fixed the WKS issue by not clobbering the heap_segment_saved_allocated Fixed a WKS specific issue by not clobbering the heap_segment_saved_allocated Jul 27, 2022
@mrsharm mrsharm marked this pull request as draft July 28, 2022 05:29
@mrsharm mrsharm marked this pull request as ready for review July 28, 2022 06:17
@mrsharm
Copy link
Member Author

mrsharm commented Jul 28, 2022

This PR is ready. I ran the benchmark numbers for the EnumToString benchmark and the numbers are back to those of segments with this change:

Method value Mean Error StdDev Median Min Max Gen 0 Allocated
EnumToString Yellow 13.31 ns 0.274 ns 0.243 ns 13.32 ns 12.90 ns 13.79 ns 0.0023 24 B

In comparison to regions without the fix:

Method value Mean Error StdDev Median Min Max Gen 0 Allocated
EnumToString Yellow 15.75 ns 0.325 ns 0.271 ns 15.70 ns 15.37 ns 16.33 ns 0.0023 24 B

As a baseline, the numbers for Segments were:

Method value Mean Error StdDev Median Min Max Gen 0 Allocated
EnumToString Yellow 13.52 ns 0.270 ns 0.226 ns 13.53 ns 13.15 ns 14.02 ns 0.0023 24 B

src/coreclr/gc/gc.cpp Outdated Show resolved Hide resolved
@mrsharm mrsharm merged commit f974d59 into dotnet:main Aug 1, 2022
@sebastienros
Copy link
Member

Can you check this PR is not the source of this issue? #73254
In the meantime, I am trying to reduce the range of commits to help us isolate the actual change.

@EgorBo
Copy link
Member

EgorBo commented Aug 4, 2022

@ghost ghost locked as resolved and limited conversation to collaborators Oct 2, 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.

4 participants