Use ValueStringBuilder to fix allocations in ToString() methods of XdsDateTime and XsdDuration

[TrayanZapryanov]

FixesIssue: 64853

Benchmark results :
Before

Method	Mean	Error	StdDev	Median	Min	Max	Gen 0	Allocated
DateTime_ToString	79.56 ns	1.238 ns	0.967 ns	79.64 ns	77.60 ns	81.20 ns	0.0486	408 B
DateTime_ToString_Local	243.25 ns	1.684 ns	1.576 ns	243.55 ns	241.02 ns	245.91 ns	0.0541	456 B
DateTime_ToString_Unspecified	76.37 ns	0.422 ns	0.394 ns	76.41 ns	75.65 ns	77.01 ns	0.0487	408 B
DateTime_ToString_RoundtripKind	78.79 ns	0.868 ns	0.769 ns	78.70 ns	77.46 ns	80.15 ns	0.0485	408 B
TimeSpan_ToString	62.96 ns	0.485 ns	0.454 ns	62.88 ns	62.09 ns	63.75 ns	0.0200	168 B

Using SpanFormattable:

Method	Mean	Error	StdDev	Median	Min	Max	Gen 0	Allocated
DateTime_ToString	129.07 ns	1.138 ns	1.009 ns	129.34 ns	125.84 ns	129.72 ns	0.0131	112 B
DateTime_ToString_Local	291.06 ns	4.934 ns	4.615 ns	288.28 ns	287.16 ns	300.48 ns	0.0139	120 B
DateTime_ToString_Unspecified	127.20 ns	1.123 ns	1.051 ns	127.49 ns	123.74 ns	127.97 ns	0.0134	112 B
DateTime_ToString_RoundtripKind	123.78 ns	0.787 ns	0.736 ns	123.85 ns	121.52 ns	124.67 ns	0.0132	112 B
TimeSpan_ToString	40.70 ns	0.105 ns	0.098 ns	40.67 ns	40.53 ns	40.88 ns	0.0066	56 B

Latest:

Method	Mean	Error	StdDev	Median	Min	Max	Gen 0	Allocated
DateTime_ToString	54.50 ns	0.304 ns	0.270 ns	54.55 ns	53.98 ns	54.99 ns	0.0096	80 B
DateTime_ToString_Local	187.15 ns	2.430 ns	2.029 ns	187.60 ns	180.93 ns	189.17 ns	0.0099	88 B
DateTime_ToString_Unspecified	49.17 ns	0.523 ns	0.464 ns	49.24 ns	47.62 ns	49.50 ns	0.0095	80 B
DateTime_ToString_RoundtripKind	53.01 ns	0.768 ns	0.718 ns	53.34 ns	51.73 ns	53.67 ns	0.0095	80 B
TimeSpan_ToString	41.16 ns	0.206 ns	0.192 ns	41.22 ns	40.55 ns	41.31 ns	0.0065	56 B

[ghost]

Tagging subscribers to this area: @dotnet/area-system-xml
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Fixes Issue: 64853

Author:	TrayanZapryanov
Assignees:	-
Labels:	area-System.Xml, community-contribution
Milestone:	-

[danmoseley]

Could you please check that the unit tests cover these paths?

[danmoseley]

Could you change the title to something descriptive? It is more useful than a number.

[TrayanZapryanov]

Could you please check that the unit tests cover these paths?

Looks like existing tests already found a problem :)

[danmoseley]

Would you mind evaluating whether there are unit tests for the areas you're changing? This code is old, and the tests are old too so there is some possibility that it is not properly tested as a regression might slip though.

[TrayanZapryanov]

Would you mind evaluating whether there are unit tests for the areas you're changing? This code is old, and the tests are old too so there is some possibility that it is not properly tested as a regression might slip though.

Once I've applied feedback changes found that there are 3850 tests in XmlConvertTests.cs . The only thing that was not able to find is if DateTimeTypeCode.GYearMonth/GYear/GMonthDay/GDay/GMonth in XsdDateTime are tested. Maybe here somebody from owners can check.

[stephentoub]

Thanks, but I didn't mean to separate it out into its own file. I meant just move the function to the primary ValueStringBuilder.cs file.

[TrayanZapryanov]

I've tried, but it was not so easy as some things stopped building.
Can we do this in another PR ?

[stephentoub]

Can we do this in another PR ?

I'd prefer we fix the problem in this PR, which would otherwise be duplicating the code for that function. What stopped building?

[TrayanZapryanov]

C:\Work\Github\TZ_runtime\src\libraries\Common\src\System\Text\ValueStringBuilder.cs(276,110): error CS0246: The type or namespace name 'ISpanFormattable' could not be found (are you missing a using directive or an assembly reference?) [C:\Work\Github\TZ_runtime\src\libraries\System.Text.RegularExpressions\gen\System.Text.RegularExpressions.Generator.csproj] System.Text.Json.SourceGeneration.Roslyn3.11 -> C:\Work\Github\TZ_runtime\artifacts\bin\System.Text.Json.SourceGeneration.Roslyn3.11\Debug\netstandard2.0\System.Text.Json.SourceGeneration.dll

[stephentoub]

Thanks. You should be able to fix that by adding this:

internalinterfaceISpanFormattable:IFormattable{boolTryFormat(Span<char>destination,outintcharsWritten,ReadOnlySpan<char>format,IFormatProvider?provider);}

here:

runtime/src/libraries/System.Text.RegularExpressions/gen/Stubs.cs

Lines 35 to 37 in 931b70a

	namespaceSystem
	{
	internalstaticclassStringExtensions

or something along those lines.

[TrayanZapryanov]

Something strange happens. When I build Release it is successful.
Command that I am executing is according build guide: build.cmd clr+libs -rc Release
But if I tri to build tests and to run them with : build.cmd -subset libs.tests -test -rc Release
Then I see errors, also if I open solutions in VS.
Next exception is:
1>C:\Work\Github\TZ_runtime\src\libraries\Common\src\System\Text\ValueStringBuilder.cs(276,110,276,126): error CS0246: The type or namespace name 'ISpanFormattable' could not be found (are you missing a using directive or an assembly reference?)
1>Done building project "System.Text.Encodings.Web.csproj" -- FAILED.

[stephentoub]

Ok, if we have src projects using ValueStringBuilder.cs and building for downlevel, which that description suggests we are, this isn't going to be trivial. Sorry for sending you down a bad path. Let's back up. Instead of moving AppendSpanFormattable into the main file, let's do something similar to what you were at one point doing: put it in its own file... but not just in System.Private.Xml... put that file next to where the main one lives in common, and then just include the file into the projects that need AppendSpanFormattable... I think that'll just be System.Private.Corelib and System.Private.Xml. Hopefully that goes better. If you still run into troubles, push up your changes to this PR and I can pull it down and fix things up.

[TrayanZapryanov]

You can check if this is what you have in mind.
Also other comments resolved - just the name of new file is not so good, but it is descriptive :)

[TrayanZapryanov]

@stephentoub One more bad news :)
Using AppendSpanFormattable is slower than original implementation as we use "D4" and "D2" format. And this goes to slow formatting of int : TryFormatInt32Slow

Here are results :
Before

Method	Mean	Error	StdDev	Median	Min	Max	Gen 0	Allocated
DateTime_ToString	79.56 ns	1.238 ns	0.967 ns	79.64 ns	77.60 ns	81.20 ns	0.0486	408 B
DateTime_ToString_Local	243.25 ns	1.684 ns	1.576 ns	243.55 ns	241.02 ns	245.91 ns	0.0541	456 B
DateTime_ToString_Unspecified	76.37 ns	0.422 ns	0.394 ns	76.41 ns	75.65 ns	77.01 ns	0.0487	408 B
DateTime_ToString_RoundtripKind	78.79 ns	0.868 ns	0.769 ns	78.70 ns	77.46 ns	80.15 ns	0.0485	408 B
TimeSpan_ToString	62.96 ns	0.485 ns	0.454 ns	62.88 ns	62.09 ns	63.75 ns	0.0200	168 B

SpanFormattable:

Method	Mean	Error	StdDev	Median	Min	Max	Gen 0	Allocated
DateTime_ToString	129.07 ns	1.138 ns	1.009 ns	129.34 ns	125.84 ns	129.72 ns	0.0131	112 B
DateTime_ToString_Local	291.06 ns	4.934 ns	4.615 ns	288.28 ns	287.16 ns	300.48 ns	0.0139	120 B
DateTime_ToString_Unspecified	127.20 ns	1.123 ns	1.051 ns	127.49 ns	123.74 ns	127.97 ns	0.0134	112 B
DateTime_ToString_RoundtripKind	123.78 ns	0.787 ns	0.736 ns	123.85 ns	121.52 ns	124.67 ns	0.0132	112 B
TimeSpan_ToString	40.70 ns	0.105 ns	0.098 ns	40.67 ns	40.53 ns	40.88 ns	0.0066	56 B

Rollback to previous formatting:

Method	Mean	Error	StdDev	Median	Min	Max	Gen 0	Allocated
DateTime_ToString	54.50 ns	0.304 ns	0.270 ns	54.55 ns	53.98 ns	54.99 ns	0.0096	80 B
DateTime_ToString_Local	187.15 ns	2.430 ns	2.029 ns	187.60 ns	180.93 ns	189.17 ns	0.0099	88 B
DateTime_ToString_Unspecified	49.17 ns	0.523 ns	0.464 ns	49.24 ns	47.62 ns	49.50 ns	0.0095	80 B
DateTime_ToString_RoundtripKind	53.01 ns	0.768 ns	0.718 ns	53.34 ns	51.73 ns	53.67 ns	0.0095	80 B
TimeSpan_ToString	41.16 ns	0.206 ns	0.192 ns	41.22 ns	40.55 ns	41.31 ns	0.0065	56 B

You can use Performance PR for benchmark

[TrayanZapryanov]

@stephentoub I've pushed changes with previous formatting for D2/4/X of integers as they are much faster. But kept changes for split of AppendSpanFormattable as they seems useful and can be used in other places. Now according benchmarks looks like we have memory + time wins. Are you okay with this changes or I should add something more ?

[TrayanZapryanov]

@danmoseley Is there something more that is expected from me ?

[danmoseley]

@stephentoub was your feedback satisfactorily addressed?

[stephentoub]

Thanks.

[TrayanZapryanov]

@danmoseley Once I have one approval, what's next? I see some build errors , but build was not so stable at this time. Should I somehow retrigger? Also who is merging this PR, me or somebody from the team?
Sorry for so stupid questions, but this is my first contribution.

[stephentoub]

Thanks, @TrayanZapryanov. Not stupid questions. We handle merging. I re-ran the failed leg and it passed. Thanks for the contribution.