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

Correct values for HighMemoryLoadThresholdBytes, MemoryLoadBytes. #32494

Merged
merged 1 commit into from
Feb 21, 2020

Conversation

naricc
Copy link
Contributor

@naricc naricc commented Feb 18, 2020

Correct values fort HighMemoryLoadThresholdBytes, MemoryLoadBytes. I looked in the code for CoreCLR and spoke with Maoni Stephens and Jan Kotas about the values used in CoreCLR, so I am confident they are all correct now.

HighMemoryThresholdBytes is a system wide memory usage threshold at which CoreCLR changes its compaction strategy. There is no direct correspondance to anything in mono-sgen. But since it used by software caches to determine when memory in the system is "nearly" exhauste, mono-sgent will use .9 * total physical memory as an acceptable value.

@BrzVlad
Copy link
Member

BrzVlad commented Feb 18, 2020

@naricc was there something left to fix for TotalAvailableMemoryBytes ? Since that was the name of the old PR

@naricc
Copy link
Contributor Author

naricc commented Feb 18, 2020

@naricc was there something left to fix for TotalAvailableMemoryBytes ? Since that was the name of the old PR

@BrzVlad
I had thought that needed fixing, but after looking at CoreCLR, it turns out it does not. It is supposed to be the total amount of physical memory in the machine, regardless if any part of it is already used. This is the value it already has in mono-sgen.

Copy link
Contributor

@CoffeeFlux CoffeeFlux left a comment

Choose a reason for hiding this comment

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

Isn't all this copy-pasted? Please put the function in a header and have it only declared once.

@naricc naricc force-pushed the naricc/GetGCMemoryInfoFix branch from 2310893 to f6c6900 Compare February 18, 2020 17:40
@naricc naricc requested a review from thaystg as a code owner February 18, 2020 17:40
@CoffeeFlux CoffeeFlux changed the title Correct values fort HighMemoryLoadThresholdBytes, MemoryLoadBytes. Correct values for HighMemoryLoadThresholdBytes, MemoryLoadBytes. Feb 18, 2020
Copy link
Contributor

@CoffeeFlux CoffeeFlux left a comment

Choose a reason for hiding this comment

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

LGTM, though if this fixes the GCMemoryInfo corefx test please re-enable that before merging and mark this PR as fixing the corresponding issue.

@naricc naricc force-pushed the naricc/GetGCMemoryInfoFix branch from f6c6900 to 8ee37bb Compare February 18, 2020 22:18
@naricc naricc force-pushed the naricc/GetGCMemoryInfoFix branch from e50f003 to 7990e38 Compare February 18, 2020 22:53
Renabled test.

Can't reenable that test yet.
@naricc naricc force-pushed the naricc/GetGCMemoryInfoFix branch from 7990e38 to 15a84e4 Compare February 21, 2020 03:24
@naricc
Copy link
Contributor Author

naricc commented Feb 21, 2020

@CoffeeFlux It seems there is still an issue with fragmentation regarding that test I will have to look into before I can re-enable it.

@naricc naricc merged commit b357a47 into dotnet:master Feb 21, 2020
@mikernet
Copy link
Contributor

mikernet commented Mar 6, 2020

Which platforms are affected by the issue this addresses and what is the nature of the issue exactly - is it completely broken, slightly off, etc..?

Are the values provided in .NET Core 3.1 running on Windows reliable for the purpose of cache management?

@CoffeeFlux
Copy link
Contributor

@mikernet this is a Mono-specific change, so it does not affect .NET Core 3.1. We only just recently added the API to Mono as part of larger .NET 5 work, so it has yet to ship to anyone yet :)

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

5 participants