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/8.0-staging] Clean up the thread local memory regardless of managed thread's presence #95722

Merged

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Dec 7, 2023

Backport of #95715 to release/8.0-staging

/cc @kunalspathak

Customer Impact

#95362 doesn't fully solve the problem of memory leak and our customer has reported that the leak still exists. I have provided the binaries that contains the fix in this PR and they confirmed that it solved the memory leak problem.

Testing

Verified it manually with the C# application mentioned in #95715.
The customer confirmed that it passed their test.

Risk

Not risky because we will free the thread local memory only during the corresponding thread's termination.

IMPORTANT: If this backport is for a servicing release, please verify that:

IMPORTANT: If this backport is for a servicing release, please verify that:

  • The PR target branch is release/X.0-staging, not release/X.0.

  • If the change touches code that ships in a NuGet package, you have added the necessary package authoring and gotten it explicitly reviewed.

@kunalspathak kunalspathak added the Servicing-consider Issue for next servicing release review label Dec 8, 2023
@kunalspathak
Copy link
Member

@JulieLeeMSFT
Copy link
Member

cc @mangod9 @kouvel please review the code for servicing fix.

@JulieLeeMSFT JulieLeeMSFT removed the Servicing-consider Issue for next servicing release review label Dec 8, 2023
t_ThreadStatics.NonGCThreadStaticBlocks = nullptr;

delete[] t_ThreadStatics.GCThreadStaticBlocks;
t_ThreadStatics.GCThreadStaticBlocks = nullptr;
Copy link
Member

Choose a reason for hiding this comment

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

believe this is just cleanup based on last PR, and doesn't affect the fix?

Copy link
Member

Choose a reason for hiding this comment

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

Correct - this addressed @AaronRobinsonMSFT's comment in #95362 (comment)

@JulieLeeMSFT JulieLeeMSFT added this to the 8.0.x milestone Dec 13, 2023
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 8.0.x

@JulieLeeMSFT JulieLeeMSFT added the Servicing-consider Issue for next servicing release review label Dec 14, 2023
@rbhanda rbhanda modified the milestones: 8.0.x, 8.0.2 Dec 14, 2023
@rbhanda rbhanda added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Dec 14, 2023
@jeffschwMSFT jeffschwMSFT merged commit 8931c13 into release/8.0-staging Jan 2, 2024
111 of 118 checks passed
@jkotas jkotas deleted the backport/pr-95715-to-release/8.0-staging branch January 2, 2024 17:25
@github-actions github-actions bot locked and limited conversation to collaborators Feb 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-VM-coreclr Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants