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

[release/7.0] Fix commit accounting for large pages #78564

Merged
merged 1 commit into from
Nov 29, 2022

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Nov 18, 2022

Backport of #78531 to release/7.0

/cc @cshung

Customer Impact

The impact of this bug is constrained to users with large pages enabled and this PR fixes the commit accounting so that we don't prematurely OOM. We discovered this issue while investigating a first party customer's out of memory (OOM) issue after upgrading to .NET 7 with regions enabled.

Testing

We were able to reproduce this issue locally and tested that with the fix, the OOM didn't occur. The first party team has additionally confirmed that we are no longer OOM'ing with the patched fix. Additionally, they have been continuing to test this for a couple of days without issue; previously their process would throw an OOM before initialization.

Risk

The risk here is constrained to where large pages are enabled, and the rest of the code path is the same. This is a fix for the aforementioned bug described above and therefore, it should reduce the risk of users getting into an OOM.

IMPORTANT: Is this backport for a servicing release? If so and this change touches code that ships in a NuGet package, please make certain that you have added any necessary package authoring and gotten it explicitly reviewed.

@ghost
Copy link

ghost commented Nov 18, 2022

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

Issue Details

Backport of #78531 to release/7.0

/cc @cshung

Customer Impact

Testing

Risk

IMPORTANT: Is this backport for a servicing release? If so and this change touches code that ships in a NuGet package, please make certain that you have added any necessary package authoring and gotten it explicitly reviewed.

Author: github-actions[bot]
Assignees: -
Labels:

area-GC-coreclr

Milestone: -

@cshung cshung marked this pull request as draft November 18, 2022 17:23
@carlossanlop
Copy link
Member

@cshung @mangod9 do we intend to include this in the January release? If yes, please make sure it's ready to merge before Nov. 29th. I see this is still a draft.

I ask because for the January release, we will only have a one day window to merge servicing PRs, so I want to make sure all the PRs are 100% ready on that day for me to just click the merge button. That window is from Nov. 29th to Nov. 30th.

Pending things to address:

  • Mark as ready for review.
  • Fill out the template.
  • Confirm CI looks good.
  • Add servicing-label.
  • Send email to Tactics requesting approval.

@mrsharm mrsharm marked this pull request as ready for review November 22, 2022 20:49
Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

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

approved. we will take for consideration in 7.0.x

@mrsharm mrsharm added the Servicing-consider Issue for next servicing release review label Nov 23, 2022
@jeffschwMSFT jeffschwMSFT added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Nov 23, 2022
@jeffschwMSFT jeffschwMSFT added this to the 7.0.2 milestone Nov 23, 2022
@mrsharm
Copy link
Member

mrsharm commented Nov 28, 2022

Hi @carlossanlop, can we get this PR merged before the merge window ends? I believe we have gone through all the aforementioned steps to ensure this is ready i.e. filled in the template and got approval. Thanks!

@carlossanlop
Copy link
Member

@mrsharm can't. We need to wait for branding to happen first.

@carlossanlop
Copy link
Member

Branding has been done. Milestone is 7.0.2. Signed-off by area owners. Approved by Tactics. No OOB package authoring changes needed. CI is green.
Ready to merge. :shipit:

@carlossanlop carlossanlop merged commit d3999cc into release/7.0 Nov 29, 2022
@carlossanlop carlossanlop deleted the backport/pr-78531-to-release/7.0 branch November 29, 2022 23:14
@ghost ghost locked as resolved and limited conversation to collaborators Dec 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-GC-coreclr Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants