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

Remove most of string.Trim() usages in System.Private.Xml solution. #75452

Merged
merged 5 commits into from Sep 26, 2022
Merged

Remove most of string.Trim() usages in System.Private.Xml solution. #75452

merged 5 commits into from Sep 26, 2022

Conversation

TrayanZapryanov
Copy link
Contributor

Remove most of string.Trim() usages in System.Private.Xml solution.
They are replaced with string.AsSpan().Trim(XmlConvert.WhitespaceChars).
If string really contains a space - this will reduce one string allocation.

Several one left, but it is hard to be replaced as code around them expects string and there is no ReadOnlySpan overload.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Sep 12, 2022
@ghost
Copy link

ghost commented Sep 12, 2022

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

Issue Details

Remove most of string.Trim() usages in System.Private.Xml solution.
They are replaced with string.AsSpan().Trim(XmlConvert.WhitespaceChars).
If string really contains a space - this will reduce one string allocation.

Several one left, but it is hard to be replaced as code around them expects string and there is no ReadOnlySpan overload.

Author: TrayanZapryanov
Assignees: -
Labels:

area-System.Xml
Milestone: -

eiriktsarpalis
eiriktsarpalis approved these changes Sep 12, 2022
View reviewed changes
Copy link
Member

@eiriktsarpalis eiriktsarpalis left a comment

Choose a reason for hiding this comment

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

LGTM superficially, although @krwq might want to take a look as well since he's more familiar with the codebase.

krwq
krwq approved these changes Sep 13, 2022
View reviewed changes
Copy link
Member

@krwq krwq left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Sep 13, 2022 •
edited

There appear to be System.Xml.RW.XmlConvert.Tests failures in CI. Could they be related to this change?

@TrayanZapryanov
Copy link
Contributor Author

There appear to be System.Xml.RW.XmlConvert.Tests failures in CI. Could they be related to this change?

@eiriktsarpalis Maybe related to #74488

@TrayanZapryanov
Copy link
Contributor Author

TrayanZapryanov commented Sep 14, 2022 •
edited

@eiriktsarpalis test fail should be connected with issue that I mentioned.
Here it is the callstack of the error :

/private/tmp/helix/working/AC830960/w/B05F097F/e /private/tmp/helix/working/AC830960/w/B05F097F/e
  Discovering: System.Xml.RW.XmlConvert.Tests (method display = ClassAndMethod, method display options = None)
  Discovered:  System.Xml.RW.XmlConvert.Tests (found 1 test case)
  Starting:    System.Xml.RW.XmlConvert.Tests (parallel test collections = on, max threads = 6)
Process terminated. Assertion failed.
   at System.Number.TryParseNumber(Char*& str, Char* strEnd, NumberStyles styles, NumberBuffer& number, NumberFormatInfo info) in /_/src/libraries/System.Private.CoreLib/src/System/Number.Parsing.cs:line 347
   at System.Number.TryStringToNumber(ReadOnlySpan`1 value, NumberStyles styles, NumberBuffer& number, NumberFormatInfo info) in /_/src/libraries/System.Private.CoreLib/src/System/Number.Parsing.cs:line 2648
   at System.Number.TryParseDouble(ReadOnlySpan`1 value, NumberStyles styles, NumberFormatInfo info, Double& result) in /_/src/libraries/System.Private.CoreLib/src/System/Number.Parsing.cs:line 2443
   at System.Number.ParseDouble(ReadOnlySpan`1 value, NumberStyles styles, NumberFormatInfo info) in /_/src/libraries/System.Private.CoreLib/src/System/Number.Parsing.cs:line 2390
   at System.Double.Parse(ReadOnlySpan`1 s, NumberStyles style, IFormatProvider provider) in /_/src/libraries/System.Private.CoreLib/src/System/Double.cs:line 405
   at System.Xml.XmlConvert.ToDouble(String s) in /_/src/libraries/System.Private.Xml/src/System/Xml/XmlConvert.cs:line 1019
   at System.Xml.Tests.ToTypeTests.TestInvalid(String[] array, String type, String[] format) in /_/src/libraries/System.Private.Xml/tests/XmlConvert/ToTypeTests.cs:line 109
   at System.Xml.Tests.ToTypeTests.TestInvalid(String[] array, String type) in /_/src/libraries/System.Private.Xml/tests/XmlConvert/ToTypeTests.cs:line 84
   at System.Xml.Tests.ToTypeTests.ToType12() in /_/src/libraries/System.Private.Xml/tests/XmlConvert/ToTypeTests.cs:line 324
   at OLEDB.Test.ModuleCore.CVariation.Execute() in /_/src/libraries/Common/tests/System/Xml/ModuleCore/cvariation.cs:line 76
   at OLEDB.Test.ModuleCore.CTestCase.<>c__DisplayClass7_1.<TestCases>b__0() in /_/src/libraries/Common/tests/System/Xml/ModuleCore/ctestcase.cs:line 131
   at OLEDB.Test.ModuleCore.XunitTestCase.Run() in /_/src/libraries/Common/tests/System/Xml/ModuleCore/XunitTestCase.cs:line 33
   at System.Xml.Tests.XmlConvertTests.RunTests(XunitTestCase testCase) in /_/src/libraries/System.Private.Xml/tests/XmlConvert/XmlConvertTests.cs:line 15
   at InvokeStub_XmlConvertTests.RunTests(Object, Object, IntPtr*)

How can I trigger tests without pushing new changes ?
Maybe it was temporary problem...
Looks like a real problem.

krwq
krwq reviewed Sep 26, 2022
View reviewed changes
src/libraries/System.Private.Xml/src/System/Xml/XmlConvert.cs
@@ -1008,6 +1010,8 @@ public static float ToSingle(string s)

public static double ToDouble(string s)
{
ArgumentNullException.ThrowIfNull(s);
Copy link
Member

Choose a reason for hiding this comment

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

interestingly this currently throws NRE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@krwq This is to mimic current behavior. Check upper comment which shows difference when using double/float.Parse(string...) and double/float.Parse(ReadOnlySpan...) and passing null.

runtime/src/libraries/System.Private.CoreLib/src/System/Single.cs

Lines 387 to 398 in b40c3a3

public static float Parse(string s, NumberStyles style, IFormatProvider? provider)
{
NumberFormatInfo.ValidateParseStyleFloatingPoint(style);
if (s == null) ThrowHelper.ThrowArgumentNullException(ExceptionArgument.s);
return Number.ParseSingle(s, style, NumberFormatInfo.GetInstance(provider));
}
public static float Parse(ReadOnlySpan<char> s, NumberStyles style = NumberStyles.Float | NumberStyles.AllowThousands, IFormatProvider? provider = null)
{
NumberFormatInfo.ValidateParseStyleFloatingPoint(style);
return Number.ParseSingle(s, style, NumberFormatInfo.GetInstance(provider));
}

@krwq krwq merged commit b34abe1 into dotnet:main Sep 26, 2022
@krwq
Copy link
Member

krwq commented Sep 26, 2022

Thanks @TrayanZapryanov! Please file an issue on #75452 (comment) if you get a chance

@TrayanZapryanov TrayanZapryanov deleted the xmlconvert_reduce_trimstring_usage branch September 30, 2022 10:51
@TrayanZapryanov TrayanZapryanov mentioned this pull request Sep 30, 2022
Closed
@dotnet dotnet locked as resolved and limited conversation to collaborators Oct 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@krwq krwq krwq approved these changes

@eiriktsarpalis eiriktsarpalis eiriktsarpalis approved these changes

Assignees
No one assigned
Labels
area-System.Xml community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@TrayanZapryanov @eiriktsarpalis @krwq