Unify CoreLib/System.Runtime.Numerics formatting/parsing code

[stephentoub]

Once upon a time it seems as though System.Runtime.Numerics forked the integer formatting/parsing code that's in CoreLib. That CoreLib code has since evolved, and is also now being mirrored to corefx, where System.Runtime.Numerics could include it rather than maintaining its own copy.

cc: @tannergooding

[nabeelvalley]

Hi, is this still open? What's needed on this?

[stephentoub]

is this still open?

Yes

What's needed on this?

The file
https://github.com/dotnet/corefx/blob/master/src/Common/src/System/Globalization/FormatProvider.Number.cs
is now a stale copy of https://github.com/dotnet/corefx/blob/master/src/Common/src/CoreLib/System/Number.Formatting.cs
though there's likely some functionality in the former that no longer exists in the latter but that's still needed for BigInteger.

The work here is to rationalize the difference and get to a point where the System.Runtime.Numerics library (which is currently including the former) can instead include the latter, and do so without negatively impacting the performance of code currently using the latter.

[danmoseley]

@nabeelvalley want to take a shot at this work?

[nabeelvalley]

@danmosemsft / @stephentoub I'd like to look at it, still a little unclear about what exactly is needed though, and also what background knowledge/info would I need to work on this?

[danmoseley]

@nabeelvalley its pretty much what's described above. You could take a look at the code mentioned maybe.

For a first contribution though, you could look at a smaller change if you want to ? There are a lot of issues marked "up for grabs" you could choose from.

[nabeelvalley]

Hi @danmosemsft , thanks, I've taken a look and it seems a bit out of my depth at the moment, but I will look at what else is around

[danmoseley]

@nabeelvalley ok, not a problem, hope to see you elsewhere in the repo!

[ghost]

Tagging subscribers to this area: @tannergooding
Notify danmosemsft if you want to be subscribed.

[ViktorHofer]

@stephentoub is that still the case with the consolidated repo?

[stephentoub]

Yup

[huoyaoyuan]

Looking at this. Number is using a lot of CoreLib internals. Though many of them isn't required by System.Runtime.Numerics, it isn't factored to be included only a part. Some others like FastAllocateString are critical to performance.
Folding it into CoreLib doesn't sound correct to me. @stephentoub any thoughts here?

[huoyaoyuan]

It does help a lot to fold into CoreLib, but it doesn't sound correct that anything numeric related are required to be in CoreLib to get best performance.

Currently blockers to compile Number out of CoreLib:

• FastAllocateString. How does ValueStringBuilder perform comparing to it?
• Decimal/Half manipulating. Can become public API, or use reinterpret cast.
• NumberFormatInfo internals. They are all available through public API, however those APIs has to defensive copy for array-returning members.
  • This also means that any third-party library can't utilize NumberFormatInfo with good performance

[stephentoub]

anything numeric related are required to be in CoreLib to get best performance

It is likely always going to be the case that internal implementation details of corelib can be utilized to eek out the best perf. For example, you mention FastAllocateString: that will never be exposed publicly, because it's an implementation detail of creating a mutable System.String that's written to and then published for immutable consumption. The public version of that is String.Create, which enables the same functionality but in a safe, span-based manner, the primary overhead of which is an extra delegate invocation.

it isn't factored to be included only a part

Work here could include refactoring to split the file into pieces compiled into both assemblies and pieces that are unique to one or the other. It could also (if feasible) involve changing portions of the BigInteger code to sit on top of public API instead where that's possible functionally and without significantly impacting performance.

[huoyaoyuan]

Yep. After looking a bit more, the only required thing is NumberFormatInfo. It's API shape is unfriendly for performance, and the number formatting code access internal fields directly to avoid defensive copy. I think it's better to open a new issue for discussing it.