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

Cache process info where possible and dispose Process objects. #11274

Merged
merged 3 commits into from
Feb 19, 2025

Conversation

Erarndt
Copy link
Contributor

@Erarndt Erarndt commented Jan 13, 2025

Fixes #

Context

There are multiple places where information about the current process is retrieved. There is a non-trivial cost to getting this information and the Process objects should be disposed to avoid additional GC cost due to the finalizer running. This change adds properties in EnvironmentUtilities that cache info such as the current process ID (this implementation is copied from .NET 6), and in some instances, updates some callers to dispose of the Process object for uncommon usages.

Changes Made

Testing

Notes

@JanKrivanek
Copy link
Member

Related to #11160

@Erarndt Erarndt marked this pull request as ready for review January 15, 2025 16:42
@SimaTian SimaTian requested a review from a team as a code owner January 20, 2025 09:14
@SimaTian SimaTian force-pushed the dev/erarndt/disposeProcess branch from 25b098e to 8a74a3e Compare January 20, 2025 09:22
@SimaTian
Copy link
Member

I tried rebasing and/or merging with main, which made a mess.
In the end, I opted for checking out main, cherrypicking the commit & then fixing one issue with some old files being re-added by it.
Review will follow.
cc: @Erarndt

Copy link
Member

@SimaTian SimaTian left a comment

Choose a reason for hiding this comment

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

I've left some comments. Overall I like it.
Ping me when ready please and I'll approve.

@YuliiaKovalova YuliiaKovalova self-requested a review January 29, 2025 10:48
Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

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

Thank you!
Overall looks ready to go!

Copy link
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

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

I was experimenting with adding these to our banned-API list and found a few uses which would benefit from a long-lived Process.GetCurrentProcess() to call other methods/properties on it. Would that cause a problem or should we consider it?

Copy link
Member

@SimaTian SimaTian left a comment

Choose a reason for hiding this comment

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

LGTM

@SimaTian
Copy link
Member

SimaTian commented Feb 18, 2025

I talked with @AR-May and this has an impact that is below what we can detect with PerfStar. At least it is definitely not a regression and since it is a neat code cleanup, I'm voting for merge.

@JanProvaznik JanProvaznik merged commit 6172e6f into dotnet:main Feb 19, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants