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

TrayanZapryanov · 2022-02-06T09:38:04Z

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

…d ToString()

ghost · 2022-02-06T09:38:11Z

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 · 2022-02-06T10:02:59Z

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

danmoseley · 2022-02-06T10:03:50Z

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

TrayanZapryanov · 2022-02-06T11:05:42Z

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

Looks like existing tests already found a problem :)

src/libraries/System.Private.Xml/src/System/Xml/Schema/XsdDateTime.cs

src/libraries/System.Private.Xml/src/System/Xml/XmlConvert.cs

src/libraries/System.Private.Xml/src/System/Xml/Schema/XsdDateTime.cs

src/libraries/System.Private.Xml/src/System/Xml/XmlConvert.cs

danmoseley · 2022-02-07T04:57:27Z

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 · 2022-02-07T08:38:20Z

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 · 2022-02-07T15:23:38Z

src/libraries/System.Private.Xml/src/Misc/ValueStringBuilder.AppendFormat.cs

+            }
+        }
+    }
+}


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.

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

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?

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

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

internal interface ISpanFormattable : IFormattable { bool TryFormat(Span<char> destination, out int charsWritten, ReadOnlySpan<char> format, IFormatProvider? provider); }

here:

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

Lines 35 to 37 in 931b70a

namespace System

{

internal static class StringExtensions

or something along those lines.

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.

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.

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 :)

@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

@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 ?

src/libraries/System.Private.Xml/src/System/Xml/Schema/XsdDateTime.cs

src/libraries/System.Private.Xml/src/System/Xml/Schema/XsdDuration.cs

…ter than AppendSpanFormattable

TrayanZapryanov · 2022-02-15T12:56:07Z

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

danmoseley · 2022-02-15T23:27:31Z

@stephentoub was your feedback satisfactorily addressed?

stephentoub · 2022-02-22T22:21:30Z

Thanks.

TrayanZapryanov · 2022-02-24T19:53:53Z

@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 · 2022-02-24T21:06:09Z

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

Trayan-Zapryanov added 2 commits February 6, 2022 11:35

Use ValueStringBuilder to fix allocations in XdsDateTime struct metho…

8c1973b

…d ToString()

Use ValueStringBuilder in XsdDuration

bb2c07a

ghost added community-contribution Indicates that the PR has been added by a community member area-System.Xml labels Feb 6, 2022

TrayanZapryanov changed the title ~~Fixing Issue 64853~~ Use ValueStringBuilder to fix allocations in ToString() methods of XdsDateTime and XsdDuration Feb 6, 2022

Fix failing tests

a541193

TrayanZapryanov mentioned this pull request Feb 6, 2022

Add initial performance tests for XmlConvert. dotnet/performance#2243

Merged

stephentoub reviewed Feb 7, 2022

View reviewed changes

danmoseley reviewed Feb 7, 2022

View reviewed changes

src/libraries/System.Private.Xml/src/System/Xml/XmlConvert.cs Outdated Show resolved Hide resolved

Address feedback

403dbc1

stephentoub reviewed Feb 7, 2022

View reviewed changes

src/libraries/System.Private.Xml/src/System/Xml/Schema/XsdDateTime.cs Outdated Show resolved Hide resolved

stephentoub reviewed Feb 7, 2022

View reviewed changes

src/libraries/System.Private.Xml/src/System/Xml/Schema/XsdDuration.cs Outdated Show resolved Hide resolved

Trayan-Zapryanov added 3 commits February 7, 2022 22:06

Address feedback 2

5524b80

few more brackets rmoved

9831011

Use previous methods for formatting into 2/4/X digits as they are fas…

b97eb34

…ter than AppendSpanFormattable

ViktorHofer mentioned this pull request Feb 8, 2022

Don't use Targets* helper properties in libs #64500

Merged

eiriktsarpalis requested a review from stephentoub February 21, 2022 15:00

stephentoub approved these changes Feb 22, 2022

View reviewed changes

stephentoub merged commit 9a050cd into dotnet:main Feb 24, 2022

TrayanZapryanov deleted the issue-64853 branch February 25, 2022 07:12

ghost locked as resolved and limited conversation to collaborators Mar 27, 2022

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

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

Uh oh!

Conversation

TrayanZapryanov commented Feb 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Feb 6, 2022

Uh oh!

danmoseley commented Feb 6, 2022

Uh oh!

danmoseley commented Feb 6, 2022

Uh oh!

TrayanZapryanov commented Feb 6, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danmoseley commented Feb 7, 2022

Uh oh!

TrayanZapryanov commented Feb 7, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TrayanZapryanov Feb 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

TrayanZapryanov commented Feb 15, 2022

Uh oh!

danmoseley commented Feb 15, 2022

Uh oh!

stephentoub commented Feb 22, 2022

Uh oh!

TrayanZapryanov commented Feb 24, 2022

Uh oh!

stephentoub commented Feb 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

TrayanZapryanov commented Feb 6, 2022 •

edited

Loading

TrayanZapryanov Feb 7, 2022 •

edited

Loading