-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Remove some unsafe usage from the float-formatting logic #121308
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
base: main
Are you sure you want to change the base?
Conversation
|
@EgorBot -amd -arm -windows_intel using System;
using BenchmarkDotNet.Attributes;
public class Benchmarks
{
public static IEnumerable<object> SingleValues => new object[]
{
float.MinValue,
float.Epsilon,
float.Pi,
12345.0f,
(float)int.MaxValue,
(float)long.MaxValue,
(float)ulong.MaxValue,
float.MaxValue
};
public static IEnumerable<object> DoubleValues => new object[]
{
double.MinValue,
double.Epsilon,
double.Pi,
12345.0,
(double)int.MaxValue,
(double)long.MaxValue,
(double)ulong.MaxValue,
double.MaxValue
};
[Benchmark]
[ArgumentsSource(nameof(SingleValues))]
[MemoryRandomization]
public string ToStringSingle(float value) => value.ToString();
[Benchmark]
[ArgumentsSource(nameof(DoubleValues))]
[MemoryRandomization]
public string ToStringDouble(double value) => value.ToString();
} |
src/libraries/System.Private.CoreLib/src/System/Number.NumberToFloatingPointBits.cs
Outdated
Show resolved
Hide resolved
|
Numbers look fine, within noise or slightly faster. I expect there's some edge cases (like the subnormal value with 767 significant digits), but those are edges that require explicit user opt-in and are already going to be very expensive; so they aren't a big concern. We can do a separate follow up for minimizing the diffs further and tweaking the code slightly to allow bounds check elision on those remaining cases. The The In both cases this is conceptually similar to |
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.
Pull Request Overview
This pull request refactors the BigInteger implementation used in floating-point number formatting by eliminating unsafe code and converting pointer-based operations to safe ref-based alternatives. The changes modernize the codebase to use C# 11+ features like InlineArrays and scoped refs.
- Replaces the
fixed uint _blocks[MaxBlockCount]with anInlineArray-basedBlocksBufferstruct to eliminate unsafe code - Changes index and bit count types from
uinttointthroughout for consistency and to avoid unnecessary casts - Converts Dragon4's pointer-based margin tracking to safe ref-based approach using
scoped refandUnsafe.AreSame
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Number.BigInteger.cs | Eliminates unsafe code by replacing fixed buffer with InlineArray, changes index/count types to int, adds Unsafe.SkipInit calls, and refactors various methods for safer code |
| Number.Dragon4.cs | Removes unsafe method signature, converts pointer-based scaledMarginHigh tracking to scoped ref with Unsafe.NullRef/AreSame patterns, eliminates unnecessary casts |
| Number.NumberToFloatingPointBits.cs | Changes bit count and precision tracking variables from uint to int for consistency with BigInteger API changes |
src/libraries/System.Private.CoreLib/src/System/Number.Dragon4.cs
Outdated
Show resolved
Hide resolved
|
@EgorBot -aws_amd -profiler using System;
using BenchmarkDotNet.Attributes;
public class Benchmarks
{
public static IEnumerable<object> SingleValues => new object[]
{
float.Epsilon,
};
[Benchmark]
[ArgumentsSource(nameof(SingleValues))]
[MemoryRandomization]
public string ToStringSingle(float value) => value.ToString();
} |
8fb8e8f to
8821c51
Compare
8821c51 to
99bb7aa
Compare
No description provided.