JSON: Add support for {ReadOnly}Memory<T> #88713

jozkee · 2023-07-12T03:40:17Z

dotnet-issue-labeler · 2023-07-12T03:40:24Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2023-07-12T03:40:40Z

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

Issue Details

Fixes #86442.

Author:	Jozkee
Assignees:	-
Labels:	`area-System.Text.Json`, `new-api-needs-documentation`
Milestone:	-

src/libraries/System.Text.Json/gen/JsonSourceGenerator.Parser.cs

src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs

src/libraries/System.Text.Json/gen/JsonSourceGenerator.Parser.cs

src/libraries/System.Text.Json/tests/Common/CollectionTests/CollectionTests.Memory.cs

src/libraries/System.Text.Json/tests/Common/JsonTestHelper.cs

src/libraries/System.Text.Json/tests/Common/CollectionTests/CollectionTests.Memory.cs

src/libraries/System.Text.Json/tests/Common/UnsupportedTypesTests.cs

src/libraries/System.Text.Json/tests/System.Text.Json.Tests/JsonTestHelper.cs

eiriktsarpalis

Other than a few minor issues this looks great 👍

src/libraries/System.Text.Json/gen/Helpers/KnownTypeSymbols.cs

Changes required

eiriktsarpalis

Thanks!

stephentoub · 2023-07-17T13:54:59Z

...ext.Json/src/System/Text/Json/Serialization/Converters/Collection/ReadOnlyMemoryConverter.cs

+            int index = state.Current.EnumeratorIndex;
+
+            JsonConverter<T> elementConverter = GetElementConverter(ref state);
+            ReadOnlySpan<T> valueSpan = value.Span;
+
+            if (elementConverter.CanUseDirectReadOrWrite && state.Current.NumberHandling == null)
+            {
+                // Fast path that avoids validation and extra indirection.
+                for (; index < valueSpan.Length; index++)
+                {
+                    elementConverter.Write(writer, valueSpan[index], options);
+                }
+            }
+            else
+            {
+                for (; index < value.Length; index++)
+                {
+                    T element = valueSpan[index];
+                    if (!elementConverter.TryWrite(writer, element, options, ref state))
+                    {
+                        state.Current.EnumeratorIndex = index;
+                        return false;
+                    }
+
+                    state.Current.EndCollectionElement();
+
+                    if (ShouldFlush(writer, ref state))
+                    {
+                        state.Current.EnumeratorIndex = ++index;
+                        return false;
+                    }
+                }
+            }
+
+            return true;


Rather than duplicating all of the code for this for Memory vs ReadOnlyMemory, could there just be a shared method they both call into?

Sounds possible, @jozkee perhaps something to follow up on in RC1?

Add support for {ReadOnly}Memory

e08255a

jozkee requested review from krwq, eiriktsarpalis and layomia July 12, 2023 03:40

dotnet-issue-labeler bot added area-System.Text.Json new-api-needs-documentation labels Jul 12, 2023

ghost assigned jozkee Jul 12, 2023

jozkee added this to the 8.0.0 milestone Jul 12, 2023

jozkee commented Jul 12, 2023

View reviewed changes

src/libraries/System.Text.Json/gen/JsonSourceGenerator.Parser.cs Outdated Show resolved Hide resolved

This was referenced Jul 12, 2023

Tracking issue for CI build timeouts #76454

Closed

LibraryImportGenerator.Unit.Tests crashed in CI #87951

Closed