Reduce allocations in VersionConverter (#55564)
authorN0D4N <50947930+N0D4N@users.noreply.github.com>
Tue, 28 Sep 2021 15:29:58 +0000 (18:29 +0300)
committerGitHub <noreply@github.com>
Tue, 28 Sep 2021 15:29:58 +0000 (08:29 -0700)
* Add more test cases for VersionConverter

* Reduce allocations in VersionConverter.Read method

* Remove allocations from VersionConverter.Write method

* Parse Version from chars, instead of parsing individual components. Assert successful formatting on write.

* Allow escaping, disallow leading and trailing whitespaces

* Hopefully fix tests for .NET Framework, add few more test cases that should fail

* Assume whitespaces can be only on the start, or only at the end, add one more test case that should fail

* Remove redundant return statements

* Elaborate on comment regarding leading and trailing whitespaces. Copy it to .NetStandard2.0 target.

* Add another test case.

src/libraries/System.Text.Json/src/Resources/Strings.resx
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/VersionConverter.cs
src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.cs
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/Value.ReadTests.cs
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/Value.WriteTests.cs

index 6485157..f76179e 100644 (file)
@@ -1,17 +1,17 @@
 <?xml version="1.0" encoding="utf-8"?>
 <root>
-  <!-- 
-    Microsoft ResX Schema 
-    
+  <!--
+    Microsoft ResX Schema
+
     Version 2.0
-    
-    The primary goals of this format is to allow a simple XML format 
-    that is mostly human readable. The generation and parsing of the 
-    various data types are done through the TypeConverter classes 
+
+    The primary goals of this format is to allow a simple XML format
+    that is mostly human readable. The generation and parsing of the
+    various data types are done through the TypeConverter classes
     associated with the data types.
-    
+
     Example:
-    
+
     ... ado.net/XML headers & schema ...
     <resheader name="resmimetype">text/microsoft-resx</resheader>
     <resheader name="version">2.0</resheader>
         <value>[base64 mime encoded string representing a byte array form of the .NET Framework object]</value>
         <comment>This is a comment</comment>
     </data>
-                
-    There are any number of "resheader" rows that contain simple 
+
+    There are any number of "resheader" rows that contain simple
     name/value pairs.
-    
-    Each data row contains a name, and value. The row also contains a 
-    type or mimetype. Type corresponds to a .NET class that support 
-    text/value conversion through the TypeConverter architecture. 
-    Classes that don't support this are serialized and stored with the 
+
+    Each data row contains a name, and value. The row also contains a
+    type or mimetype. Type corresponds to a .NET class that support
+    text/value conversion through the TypeConverter architecture.
+    Classes that don't support this are serialized and stored with the
     mimetype set.
-    
-    The mimetype is used for serialized objects, and tells the 
-    ResXResourceReader how to depersist the object. This is currently not 
+
+    The mimetype is used for serialized objects, and tells the
+    ResXResourceReader how to depersist the object. This is currently not
     extensible. For a given mimetype the value must be set accordingly:
-    
-    Note - application/x-microsoft.net.object.binary.base64 is the format 
-    that the ResXResourceWriter will generate, however the reader can 
+
+    Note - application/x-microsoft.net.object.binary.base64 is the format
+    that the ResXResourceWriter will generate, however the reader can
     read any of the formats listed below.
-    
+
     mimetype: application/x-microsoft.net.object.binary.base64
-    value   : The object must be serialized with 
+    value   : The object must be serialized with
             : System.Runtime.Serialization.Formatters.Binary.BinaryFormatter
             : and then encoded with base64 encoding.
-    
+
     mimetype: application/x-microsoft.net.object.soap.base64
-    value   : The object must be serialized with 
+    value   : The object must be serialized with
             : System.Runtime.Serialization.Formatters.Soap.SoapFormatter
             : and then encoded with base64 encoding.
 
     mimetype: application/x-microsoft.net.object.bytearray.base64
-    value   : The object must be serialized into a byte array 
+    value   : The object must be serialized into a byte array
             : using a System.ComponentModel.TypeConverter
             : and then encoded with base64 encoding.
     -->
   <data name="FormatGuid" xml:space="preserve">
     <value>The JSON value is not in a supported Guid format.</value>
   </data>
+  <data name="FormatVersion" xml:space="preserve">
+    <value>The JSON value is not in a supported Version format.</value>
+  </data>
   <data name="ExpectedStartOfPropertyOrValueAfterComment" xml:space="preserve">
     <value>'{0}' is an invalid start of a property name or value, after a comment.</value>
   </data>
index 64f4986..f4bd6df 100644 (file)
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
+using System.Buffers;
+using System.Buffers.Text;
+using System.Diagnostics;
+
 namespace System.Text.Json.Serialization.Converters
 {
     internal sealed class VersionConverter : JsonConverter<Version>
     {
+#if BUILDING_INBOX_LIBRARY
+        private const int MinimumVersionLength = 3; // 0.0
+
+        private const int MaximumVersionLength = 43; // 2147483647.2147483647.2147483647.2147483647
+
+        private const int MaximumEscapedVersionLength = JsonConstants.MaxExpansionFactorWhileEscaping * MaximumVersionLength;
+#endif
+
         public override Version Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
         {
+            if (reader.TokenType != JsonTokenType.String)
+            {
+                throw ThrowHelper.GetInvalidOperationException_ExpectedString(reader.TokenType);
+            }
+
+#if BUILDING_INBOX_LIBRARY
+            bool isEscaped = reader._stringHasEscaping;
+
+            int maxLength = isEscaped ? MaximumEscapedVersionLength : MaximumVersionLength;
+            ReadOnlySpan<byte> source = stackalloc byte[0];
+            if (reader.HasValueSequence)
+            {
+                if (!JsonHelpers.IsInRangeInclusive(reader.ValueSequence.Length, MinimumVersionLength, maxLength))
+                {
+                    throw ThrowHelper.GetFormatException(DataType.Version);
+                }
+
+                Span<byte> stackSpan = stackalloc byte[isEscaped ? MaximumEscapedVersionLength : MaximumVersionLength];
+                reader.ValueSequence.CopyTo(stackSpan);
+                source = stackSpan.Slice(0, (int)reader.ValueSequence.Length);
+            }
+            else
+            {
+                source = reader.ValueSpan;
+
+                if (!JsonHelpers.IsInRangeInclusive(source.Length, MinimumVersionLength, maxLength))
+                {
+                    throw ThrowHelper.GetFormatException(DataType.Version);
+                }
+            }
+
+            if (isEscaped)
+            {
+                int backslash = source.IndexOf(JsonConstants.BackSlash);
+                Debug.Assert(backslash != -1);
+
+                Span<byte> sourceUnescaped = stackalloc byte[MaximumEscapedVersionLength];
+
+                JsonReaderHelper.Unescape(source, sourceUnescaped, backslash, out int written);
+                Debug.Assert(written > 0);
+
+                source = sourceUnescaped.Slice(0, written);
+                Debug.Assert(!source.IsEmpty);
+            }
+
+            byte firstChar = source[0];
+            byte lastChar = source[source.Length - 1];
+            if (!JsonHelpers.IsDigit(firstChar) || !JsonHelpers.IsDigit(lastChar))
+            {
+                // Since leading and trailing whitespaces are forbidden throughout System.Text.Json converters
+                // we need to make sure that our input doesn't have them,
+                // and if it has - we need to throw, to match behaviour of other converters
+                // since Version.TryParse allows them and silently parses input to Version
+                throw ThrowHelper.GetFormatException(DataType.Version);
+            }
+
+            Span<char> charBuffer = stackalloc char[MaximumVersionLength];
+            int writtenChars = JsonReaderHelper.s_utf8Encoding.GetChars(source, charBuffer);
+            if (Version.TryParse(charBuffer.Slice(0, writtenChars), out Version? result))
+            {
+                return result;
+            }
+#else
             string? versionString = reader.GetString();
+            if (!string.IsNullOrEmpty(versionString) && (!char.IsDigit(versionString[0]) || !char.IsDigit(versionString[versionString.Length - 1])))
+            {
+                // Since leading and trailing whitespaces are forbidden throughout System.Text.Json converters
+                // we need to make sure that our input doesn't have them,
+                // and if it has - we need to throw, to match behaviour of other converters
+                // since Version.TryParse allows them and silently parses input to Version
+                throw ThrowHelper.GetFormatException(DataType.Version);
+            }
             if (Version.TryParse(versionString, out Version? result))
             {
                 return result;
             }
-
+#endif
             ThrowHelper.ThrowJsonException();
             return null;
         }
 
         public override void Write(Utf8JsonWriter writer, Version value, JsonSerializerOptions options)
         {
+#if BUILDING_INBOX_LIBRARY
+            Span<char> span = stackalloc char[MaximumVersionLength];
+            bool formattedSuccessfully = value.TryFormat(span, out int charsWritten);
+            Debug.Assert(formattedSuccessfully && charsWritten >= MinimumVersionLength);
+            writer.WriteStringValue(span.Slice(0, charsWritten));
+#else
             writer.WriteStringValue(value.ToString());
+#endif
         }
     }
 }
index 96296fa..092588c 100644 (file)
@@ -645,6 +645,9 @@ namespace System.Text.Json
                 case DataType.Guid:
                     message = SR.FormatGuid;
                     break;
+                case DataType.Version:
+                    message = SR.FormatVersion;
+                    break;
                 default:
                     Debug.Fail($"The DateType enum value: {dateType} is not part of the switch. Add the appropriate case and exception message.");
                     break;
@@ -729,5 +732,6 @@ namespace System.Text.Json
         TimeSpan,
         Base64String,
         Guid,
+        Version,
     }
 }
index 3bfb3c9..de711c2 100644 (file)
@@ -309,6 +309,8 @@ namespace System.Text.Json.Serialization.Tests
             Assert.Throws<JsonException>(() => JsonSerializer.Deserialize<TimeSpan>(unexpectedString));
             Assert.Throws<JsonException>(() => JsonSerializer.Deserialize<TimeSpan?>(unexpectedString));
 
+            Assert.Throws<JsonException>(() => JsonSerializer.Deserialize<Version>(unexpectedString));
+
             Assert.Throws<JsonException>(() => JsonSerializer.Deserialize<string>("1"));
 
             Assert.Throws<JsonException>(() => JsonSerializer.Deserialize<char>("1"));
@@ -318,24 +320,53 @@ namespace System.Text.Json.Serialization.Tests
             Assert.Throws<JsonException>(() => JsonSerializer.Deserialize<Enum>(unexpectedString));
         }
 
-        [Fact]
-        public static void ReadVersion()
+        [Theory]
+        [InlineData("1.2")]
+        [InlineData("\\u0031\\u002e\\u0032", "1.2")]
+        [InlineData("1.2.3")]
+        [InlineData("\\u0031\\u002e\\u0032\\u002e\\u0033", "1.2.3")]
+        [InlineData("1.2.3.4")]
+        [InlineData("\\u0031\\u002e\\u0032\\u002e\\u0033\\u002e\\u0034", "1.2.3.4")]
+        [InlineData("2147483647.2147483647")]
+        [InlineData("\\u0032\\u0031\\u0034\\u0037\\u0034\\u0038\\u0033\\u0036\\u0034\\u0037\\u002e\\u0032\\u0031\\u0034\\u0037\\u0034\\u0038\\u0033\\u0036\\u0034\\u0037",
+            "2147483647.2147483647")]
+        [InlineData("2147483647.2147483647.2147483647")]
+        [InlineData("\\u0032\\u0031\\u0034\\u0037\\u0034\\u0038\\u0033\\u0036\\u0034\\u0037\\u002e\\u0032\\u0031\\u0034\\u0037\\u0034\\u0038\\u0033\\u0036\\u0034\\u0037\\u002e\\u0032\\u0031\\u0034\\u0037\\u0034\\u0038\\u0033\\u0036\\u0034\\u0037",
+            "2147483647.2147483647.2147483647")]
+        [InlineData("2147483647.2147483647.2147483647.2147483647")]
+        [InlineData("\\u0032\\u0031\\u0034\\u0037\\u0034\\u0038\\u0033\\u0036\\u0034\\u0037\\u002e\\u0032\\u0031\\u0034\\u0037\\u0034\\u0038\\u0033\\u0036\\u0034\\u0037\\u002e\\u0032\\u0031\\u0034\\u0037\\u0034\\u0038\\u0033\\u0036\\u0034\\u0037\\u002e\\u0032\\u0031\\u0034\\u0037\\u0034\\u0038\\u0033\\u0036\\u0034\\u0037",
+            "2147483647.2147483647.2147483647.2147483647")]
+        public static void Version_Read_Success(string json, string? actual = null)
         {
-            Version version;
-
-            version = JsonSerializer.Deserialize<Version>(@"""1.2""");
-            Assert.Equal(new Version(1, 2), version);
+            actual ??= json;
+            Version value = JsonSerializer.Deserialize<Version>($"\"{json}\"");
 
-            version = JsonSerializer.Deserialize<Version>(@"""1.2.3""");
-            Assert.Equal(new Version(1, 2, 3), version);
-
-            version = JsonSerializer.Deserialize<Version>(@"""1.2.3.4""");
-            Assert.Equal(new Version(1, 2, 3, 4), version);
+            Assert.Equal(Version.Parse(actual), value);
+        }
 
-            version = JsonSerializer.Deserialize<Version>("null");
-            Assert.Null(version);
+        [Theory]
+        [InlineData("")]
+        [InlineData("     ")]
+        [InlineData(" ")]
+        [InlineData("2147483648.2147483648.2147483648.2147483648")] //int.MaxValue + 1
+        [InlineData("2147483647.2147483647.2147483647.21474836477")] // Slightly bigger in size than max length of Version
+        [InlineData("-2147483648.-2147483648")]
+        [InlineData("-2147483648.-2147483648.-2147483648")]
+        [InlineData("-2147483648.-2147483648.-2147483648.-2147483648")]
+        [InlineData("1.-1")]
+        [InlineData("1")]
+        [InlineData("   1.2.3.4")] //Valid but has leading whitespace
+        [InlineData("1.2.3.4    ")] //Valid but has trailing whitespace
+        [InlineData("  1.2.3.4  ")] //Valid but has trailing and leading whitespaces
+        [InlineData("{}", false)]
+        [InlineData("[]", false)]
+        [InlineData("true", false)]
+        public static void Version_Read_Failure(string json, bool addQuotes = true)
+        {
+            if (addQuotes)
+                json = $"\"{json}\"";
 
-            Assert.Throws<JsonException>(() => JsonSerializer.Deserialize<Version>(@""""""));
+            Assert.Throws<JsonException>(() => JsonSerializer.Deserialize<Version>(json));
         }
 
         [Fact]
index f3e6bd8..4f1121a 100644 (file)
@@ -111,6 +111,21 @@ namespace System.Text.Json.Serialization.Tests
                 Version version = new Version(1, 2, 3, 4);
                 Assert.Equal(@"""1.2.3.4""", JsonSerializer.Serialize(version));
             }
+
+            {
+                Version version = new Version(int.MaxValue, int.MaxValue);
+                Assert.Equal(@"""2147483647.2147483647""", JsonSerializer.Serialize(version));
+            }
+
+            {
+                Version version = new Version(int.MaxValue, int.MaxValue, int.MaxValue);
+                Assert.Equal(@"""2147483647.2147483647.2147483647""", JsonSerializer.Serialize(version));
+            }
+
+            {
+                Version version = new Version(int.MaxValue, int.MaxValue, int.MaxValue, int.MaxValue);
+                Assert.Equal(@"""2147483647.2147483647.2147483647.2147483647""", JsonSerializer.Serialize(version));
+            }
         }
 
         [Theory]