-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Add option GCTrimYoungestKeepPercent to specifies the percent of youngest gen to keep during trimming #109863
base: main
Are you sure you want to change the base?
Conversation
Tagging subscribers to this area: @dotnet/gc |
const char *embeddedValue = nullptr; | ||
if (GetEmbeddedVariable(&g_compilerEmbeddedKnobsBlob, name, false, &embeddedValue)) | ||
{ | ||
*pValue = strtod(embeddedValue, NULL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Do we want to support all formats
strtod
supports? - What if the
embeddedValue
is not a valid floating point number?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will take care of error handling
*pValue = strtod(embeddedValue, NULL); | |
char *endptr; | |
double val = strtod(embeddedValue, &endptr); | |
if (embeddedValue == endptr) return false; // invalid input; no conversion was performed | |
*pValue = val; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Do we want to support all formats
strtod
supports?- What if the
embeddedValue
is not a valid floating point number?
Thanks for the review! Do I also need to add error handling for strtoull
in ReadConfigValue
and ReadKnobUInt64Value
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will take care of error handling
Thanks for your advice!
uint64_t uiValue; | ||
if (g_pRhConfig->ReadConfigValue(privateKey, &uiValue)) | ||
{ | ||
memcpy(value, &uiValue, sizeof(double)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Intentionally reinterpret_cast a 64 bit unsigned integer to a double?
I am not sure that floating point granularity is required here. Since this increases complexity can you please explain the use case? |
Thank you for review. We tested various options, for some applications for Tizen there is a difference in results between 2 and 3 and 2.25 |
Another option would be to scale the initial value up but still divide by an integer. Beyond the review/maintenance, I think another question would be whether we end up with confusion about whether different variables are ints vs floats. To some extent, my "scale up" comment has a similar problem (i.e., which values are scaled and which aren't?) -- this could be addressed by having the computation be |
we definitely don't need to add floating point support to our configs just to make this work, you can specify this in percent (or permil even, if you need more precision). so a lot of these changes aren't needed. but before we consider adding a config, let's step back as @mangod9 said and look at the user case - when you said
what exactly causes the difference? are these your applications or someone else's? are you going to be testing again to see if a slightly different number works better in the future? |
Sorry for the late reply, I was busy with other tasks.
Yes, thank you, this solution is also suitable for us.
Difference is related to how the divider value affects the application startup. |
…gest gen to keep during trimming
1d84603
to
8d4aef9
Compare
A new solution has been added that uses percentages instead of a floating-point configuration. |
Memory footprint GC latency level allows to reduce memory consumption, but might have effect on application startup time.
This PR adds new config that specifies the percent of youngest gen to keep during trimming, which allows to balance aggressiveness of trimming and effect on startup time.
An example of how this can be used:
DOTNET_GCLatencyLevel=0
DOTNET_GCTrimYoungestKeepPercent=0xF
cc @gbalykov