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.

Sign up for GitHub

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

Jump to bottom

Fix potential 'Floating Point Exception' with speculative execution #93517

Merged
merged 6 commits into from
Jan 31, 2024
Merged

Fix potential 'Floating Point Exception' with speculative execution #93517

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
89fb671
Fix potential 'Floating Point Exception' with speculative execution
grospelliergilles Oct 14, 2023
be2d0e2
Rewrite the calculation of 'generation_allocator_efficiency' and 'gen…
grospelliergilles Nov 7, 2023
e601d36
Use 'uint64_t' instead of 'size_t' for computing generation fragmenta…
grospelliergilles Dec 12, 2023
fad395d
Use 'size_t' instead of 'uint64_t' for return type.
grospelliergilles Dec 13, 2023
a2d83a5
Improve readability of the test
grospelliergilles Dec 14, 2023
ab750d4
Remove cast from 'size_t' to 'int' when using 'generation_allocator_e…
grospelliergilles Dec 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/coreclr/gc/gc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3418,7 +3418,7 @@ gc_heap::dt_high_frag_p (gc_tuning_point tp,
}
dprintf (GTC_LOG, ("h%d: gen%d, frag is %zd, alloc effi: %d%%, unusable frag is %zd, ratio is %d",
heap_number, gen_number, dd_fragmentation (dd),
(int)(100*generation_allocator_efficiency (generation_of (gen_number))),
(int)(generation_allocator_efficiency_percent (generation_of (gen_number))),
jkotas marked this conversation as resolved.
Show resolved Hide resolved
fr, (int)(fragmentation_burden*100)));
}
break;
Expand Down Expand Up @@ -22420,7 +22420,7 @@ void gc_heap::gc1()
}
}

get_gc_data_per_heap()->maxgen_size_info.running_free_list_efficiency = (uint32_t)(generation_allocator_efficiency (generation_of (max_generation)) * 100);
get_gc_data_per_heap()->maxgen_size_info.running_free_list_efficiency = (uint32_t)(generation_allocator_efficiency_percent (generation_of (max_generation)));

free_list_info (max_generation, "after computing new dynamic data");
}
Expand Down Expand Up @@ -33143,7 +33143,7 @@ void gc_heap::plan_phase (int condemned_gen_number)
if ((free_list_allocated + rejected_free_space) != 0)
free_list_efficiency = (int)(((float) (free_list_allocated) / (float)(free_list_allocated + rejected_free_space)) * (float)100);

int running_free_list_efficiency = (int)(generation_allocator_efficiency(older_gen)*100);
int running_free_list_efficiency = (int)(generation_allocator_efficiency_percent(older_gen));

dprintf (1, ("gen%d free list alloc effi: %d%%, current effi: %d%%",
older_gen->gen_num,
Expand Down
27 changes: 14 additions & 13 deletions src/coreclr/gc/gcpriv.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1024,7 +1024,7 @@ class generation
allocator free_list_allocator;

// The following fields are maintained in the older generation we allocate into, and they are only for diagnostics
// except free_list_allocated which is currently used in generation_allocator_efficiency.
// except free_list_allocated which is currently used in generation_allocator_efficiency_percent.
//
// If we rearrange regions between heaps, we will no longer have valid values for these fields unless we just merge
// regions from multiple heaps into one, in which case we can simply combine the values from all heaps.
Expand Down Expand Up @@ -5105,23 +5105,24 @@ size_t& generation_allocated_since_last_pin (generation* inst)
#endif //FREE_USAGE_STATS

inline
float generation_allocator_efficiency (generation* inst)
size_t generation_allocator_efficiency_percent (generation* inst)
{
// Because of speculative execution, it is not always safe to do the following code if sum is equal to zero
// if (sum!=0.0f)
// return ... / sum;
// To prevent this, add a small value to the divider.
// It will not change the result if sum is not zero because in this
// case sum>1.0 and precision of float is limited to 7 digits

float sum = (float)(generation_free_list_allocated (inst) + generation_free_obj_space (inst));
return ((float) (generation_free_list_allocated (inst))) / (sum + 1.0e-10f);
size_t free_obj_space = generation_free_obj_space (inst);
size_t free_list_allocated = generation_free_list_allocated (inst);
if (free_list_allocated==0)
return 0;
return (100 * free_list_allocated) / (free_list_allocated + free_obj_space);
jkotas marked this conversation as resolved.
Show resolved Hide resolved
}

inline
size_t generation_unusable_fragmentation (generation* inst)
{
return (size_t)(generation_free_obj_space (inst) +
(1.0f-generation_allocator_efficiency(inst))*generation_free_list_space (inst));
size_t free_obj_space = generation_free_obj_space (inst);
size_t free_list_allocated = generation_free_list_allocated (inst);
size_t free_list_space = generation_free_list_space (inst);
if (free_obj_space==0)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (free_obj_space==0)
if ((free_list_allocated + free_obj_space) == 0)

Copy link
Member

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.

Copy link
Contributor Author

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 type size_t and so can not be negative.

Copy link
Member

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.

Copy link
Contributor Author

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.

return 0;
return (free_obj_space + (free_obj_space * free_list_space) / (free_list_allocated + free_obj_space));
Copy link
Member

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 or inefficiency * free_list_space, being careful in both cases to minimize the impact of the integer division.

Copy link
Contributor Author

@grospelliergilles grospelliergilles Dec 13, 2023 •
edited
Loading

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.

Copy link
Member

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.

}

#define plug_skew sizeof(ObjHeader)
Expand Down
Loading