Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Fix potential 'Floating Point Exception' with speculative execution #93517
Fix potential 'Floating Point Exception' with speculative execution #93517
Changes from 3 commits
89fb671
be2d0e2
e601d36
fad395d
a2d83a5
ab750d4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
I think the idea is that if free_obj_space is zero, then the computation will be zero regardless of the free_list_allocated value. (unless one can be negative...) It's subtle and perhaps not worth the microoptimization or worth a comment. The same patterns occurs in the efficiency method.
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.
Yes if
free_obj_space
is zero then the result is always zero. The operands are of typesize_t
and so can not be negative.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.
yes, I got the idea. but this kind of microoptimization is not worth it at all. I'd much rather have the code be easier to understand.
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.
Thanks for your report. I will do as requested.
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 could use a comment, either that it is based on rewriting of
(100 - efficiency)/100 * free_list_space
orinefficiency * free_list_space
, being careful in both cases to minimize the impact of the integer division.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.
Thanks for your report. I will make the required change and add the information your want.
If you prefer I revert to the first commit (using FLT_EPSILON) I can also do that.
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.
I would really rather to not have to have the presence of FLT_EPSILON in the GC code at all if I can avoid it :) it's just another thing that someone who's not familiar with FP has to care about.
but I agree with @markples that these should continue to return size_t, instead of uint64_t. I would also add a comment above
generation_allocator_efficiency_percent
to explain why this is written this way (to accommodate this exception specific to FP which is not a common knowledge AFAIK) instead of the previous way which seems more natural.