Use Utf8JsonReader in DotNetDispatcher #2061

pranavkm · 2019-07-19T14:37:51Z

No description provided.

pranavkm · 2019-07-19T14:38:29Z

BrennanConroy · 2019-07-19T15:46:39Z

Numbers?

BrennanConroy · 2019-07-19T20:12:16Z

src/JSInterop/Microsoft.JSInterop/src/DotNetDispatcher.cs

-            var jsonDocument = JsonDocument.Parse(argsJson);
-            var shouldDisposeJsonDocument = true;
-            try
+            var utf8JsonBytes = Encoding.UTF8.GetBytes(argsJson).AsSpan();


You should be able to avoid this allocation by stackalloc/renting and using
https://github.com/dotnet/corefx/blob/7431e4938c461f6e24cf3122c0b9ca4eb27e4dde/src/Common/src/CoreLib/System/Text/Unicode/Utf8.cs#L42

This project targets netstandard2.0. I don't that API exists?

Or even better
https://github.com/dotnet/corefx/blob/7431e4938c461f6e24cf3122c0b9ca4eb27e4dde/src/Common/src/CoreLib/System/Text/UTF8Encoding.cs#L371

Is cross-compiling/multi-targeting feasible? If perf/allocation is super critical, maybe use the char* overload for Encoding.UTF8 on older platforms and the span one on newer ones.

@pranavkm We can stackalloc on netstandard2.0, it just has to be done inside a method marked as unsafe. I think we'd be OK with the "unsafe" for now (this will move to netstandard2.1 later). However I'm not sure if additional checks are needed to avoid problems if we receive an especially long string. Brennan's other suggestion may be better!

RuntimeHelpers.EnsureSufficientExecutionStack() or one of its friends will do I think

ahsonkhan · 2019-07-19T21:02:05Z

src/JSInterop/Microsoft.JSInterop/src/DotNetDispatcher.cs

+            while (reader.Read() && reader.TokenType != JsonTokenType.EndArray)
+            {
+                suppliedArgsLength++;
+                reader.Skip();


nit: I think this can be written a bit differently to avoid the double parsing of the json data.

Currently we are doing one pass to validate, and then the other to do the actual work.

I agree, this does seem a bit odd

Also maybe I'm misunderstanding the APIs, but does reader.Skip skip over complex objects or does it just skip over individual tokens? How many tokens does it think are in the input ["a", ["b1", "b2"]], or is that sort of thing not possible here?

reader.Skip skips over complex objects (not individual tokens).

So, if your reader was positioned at StartArray (or Object), it would skip to EndArray (or Object). If your reader was positioned at String, or other primitive token (like Null, True, False, Number), it wouldn't do anything.

ahsonkhan · 2019-07-19T21:02:44Z

src/JSInterop/Microsoft.JSInterop/src/DotNetDispatcher.cs

-                    }
+            // Reset the reader
+            reader = new Utf8JsonReader(utf8JsonBytes);
+            reader.Read();


nit: Add Debug.Assert(reader.TokenType == JsonTokenType.StartArray)

ahsonkhan · 2019-07-19T21:05:34Z

src/JSInterop/Microsoft.JSInterop/src/DotNetDispatcher.cs

+                    objectRefReader.TokenType == JsonTokenType.PropertyName &&
+                    objectRefReader.ValueTextEquals(DotNetObjectRefKey.EncodedUtf8Bytes))
+                {
+                    // The JSON payload looks has the expected shape.


nit: This comment is a bit confusing to read.

ahsonkhan · 2019-07-19T21:07:08Z

src/JSInterop/Microsoft.JSInterop/src/DotNetDispatcher.cs

-            static bool IsIncorrectDotNetObjectRefUse(JsonElement item, Type parameterType)
+            return suppliedArgs;
+
+            static bool IsIncorrectDotNetObjectRefUse(Type parameterType, Span<byte> utf8JsonBytes, JsonReaderState readerState)


Why not pass the reader directly rather than passing in the data/state? If you don't want the mutations to the reader be retained, pass by value (rather than ref).

~~Also it might be nice to make this not be a local method to make it super clear that we're not expecting to capture any locals in a closure, since that would be a bad perf situation.~~ Oh, never mind, it's static anyway.

src/JSInterop/Microsoft.JSInterop/src/DotNetDispatcher.cs

src/JSInterop/Microsoft.JSInterop/src/JSRuntimeBase.cs

javiercn

Overall looks good!

Now that we are doing more lower level parsing I would like to see more tests with bad input to ensure that we fail in a reasonable way. Could you add a few tests like those?

javiercn · 2019-07-20T13:48:14Z

src/JSInterop/Microsoft.JSInterop/src/DotNetDispatcher.cs

-            var jsonDocument = JsonDocument.Parse(argsJson);
-            var shouldDisposeJsonDocument = true;
-            try
+            var utf8JsonBytes = Encoding.UTF8.GetBytes(argsJson).AsSpan();


RuntimeHelpers.EnsureSufficientExecutionStack() or one of its friends will do I think

SteveSandersonMS · 2019-07-20T16:59:59Z

src/JSInterop/Microsoft.JSInterop/src/DotNetDispatcher.cs

        /// </exception>
        public static void EndInvoke(string arguments)
        {
-            var parsedArgs = ParseArguments(


I think the logic here could definitely use some extra comments now. Something to depict what shape of data we're expecting to receive here, like:

// arguments must be a valid JSON string of the form: // [taskId, success, result] // where: // - 'taskId' is a number (parsed as long) // - 'success' is a boolean // - 'result' must be JSON-deserializable as whatever .NET type T was originally specified on InvokeAsync<T>

src/JSInterop/Microsoft.JSInterop/src/JSRuntimeBase.cs

SteveSandersonMS

This looks like it will be excellent.

The CR comments from others are pretty high-quality and worth addressing, I'd say.

pranavkm · 2019-08-01T00:17:40Z

Some numbers:

	Before	After
`string`
`byte[]`
`object[]`		N/A

The byte[] allocation is a wash at this point and using stackalloc saves about 100 bytes \ JS operation (100 kb over 2k operations). It doesn't seem particularly worthwhile considering considering you have to cross-compile. Particularly given that the plan is to transmit the args as a utf8 byte array (dotnet/aspnetcore#12280), the added complexity doesn't seem worthwhile at this point.

BrennanConroy

Looks good, make sure some Blazor folks have looked at it

BrennanConroy · 2019-08-03T00:01:05Z

src/JSInterop/Microsoft.JSInterop/src/DotNetDispatcher.cs

+            else
+            {
+                assembly = loadedAssemblies.FirstOrDefault(a => new AssemblyKey(a).Equals(assemblyKey));
+


Suggested change

BrennanConroy · 2019-08-03T00:01:20Z

src/JSInterop/Microsoft.JSInterop/src/DotNetDispatcher.cs

-            static bool IsIncorrectDotNetObjectRefUse(JsonElement item, Type parameterType)
+            return suppliedArgs;
+
+            // Note that the JsonReader instance is intentionally not passed by ref (or asn in parameter) since we want a copy of the original reader.


Suggested change

// Note that the JsonReader instance is intentionally not passed by ref (or asn in parameter) since we want a copy of the original reader.

// Note that the JsonReader instance is intentionally not passed by ref (or an in parameter) since we want a copy of the original reader.

BrennanConroy · 2019-08-03T00:08:14Z

src/JSInterop/Microsoft.JSInterop/test/DotNetDispatcherTest.cs

+        }
+
+        [Fact]
+        public void ParseArguments_UsesStackForSmallJsonPayloads()


I don't understand this one

BrennanConroy · 2019-08-05T21:45:21Z

src/JSInterop/Microsoft.JSInterop/test/DotNetDispatcherTest.cs

+        }
+
+        [Fact]
+        public void ParseEndInvokeArguments_UsesStackForSmallPayloads()


Same question, what does this mean?

Is it a remnant from wanting to test the stackalloc stuff?

Yup. Removed in the update

SteveSandersonMS · 2019-08-06T13:56:29Z

src/JSInterop/Microsoft.JSInterop/src/DotNetDispatcher.cs

            return result;
        }

+        internal static bool WorkAroundTestBug;


I don't understand this. Am I missing something, or is its value always true? If so, why do we even have this flag and the if/else case below? Why not just always do L378?

Oh wait, I see how it only gets set to true in tests.

Still, is there any disadvantage in always using the logic on L378?

Not particularly. I didn't think it was necessary to change product code, but in the most ordinary cases, you're unlikely to see more than two instances of the same assembly so this doesn't change much.

Fixes dotnet/aspnetcore#10988

pranavkm · 2019-08-06T17:55:30Z

@SteveSandersonMS \ @javiercn would you like to have another look at this before I merge it in?

SteveSandersonMS

Thanks for the updates!

* Use Utf8JsonReader in DotNetDispatcher Fixes #10988 \n\nCommit migrated from dotnet/extensions@c24711c

pranavkm requested review from BrennanConroy and javiercn July 19, 2019 14:37

pranavkm requested review from SteveSandersonMS and rynowak July 19, 2019 14:38

BrennanConroy added the area-mvc label Jul 19, 2019

BrennanConroy reviewed Jul 19, 2019

View reviewed changes

ahsonkhan reviewed Jul 19, 2019

View reviewed changes

javiercn reviewed Jul 20, 2019

View reviewed changes

SteveSandersonMS reviewed Jul 20, 2019

View reviewed changes

src/JSInterop/Microsoft.JSInterop/src/JSRuntimeBase.cs Outdated Show resolved Hide resolved

SteveSandersonMS self-requested a review July 20, 2019 17:16

SteveSandersonMS reviewed Jul 20, 2019

View reviewed changes

pranavkm force-pushed the prkrishn/json-converter branch from 1178415 to 0cf3c01 Compare July 24, 2019 02:39

pranavkm force-pushed the prkrishn/json-reader branch 2 times, most recently from 10bbd81 to 545f155 Compare July 31, 2019 23:53

pranavkm changed the base branch from prkrishn/json-converter to release/3.0 July 31, 2019 23:53

pranavkm force-pushed the prkrishn/json-reader branch from 545f155 to 50ebe23 Compare July 31, 2019 23:56

pranavkm force-pushed the prkrishn/json-reader branch from 50ebe23 to 3444904 Compare August 1, 2019 00:18

pranavkm added tell-mode and removed area-mvc labels Aug 1, 2019

pranavkm added this to the 3.0.0-preview9 milestone Aug 1, 2019

BrennanConroy approved these changes Aug 3, 2019

View reviewed changes

BrennanConroy reviewed Aug 5, 2019

View reviewed changes

SteveSandersonMS reviewed Aug 6, 2019

View reviewed changes

pranavkm added 2 commits August 6, 2019 09:30

Use Utf8JsonReader in DotNetDispatcher

0945d9a

Fixes dotnet/aspnetcore#10988

Workaround test infrastructure bug

16ed80f

pranavkm added 2 commits August 6, 2019 09:30

Changes per PR

8a77082

Remove test

39af1df

Cleanup test workaround

278f0e4

pranavkm force-pushed the prkrishn/json-reader branch from 43d6c8c to 278f0e4 Compare August 6, 2019 22:10

SteveSandersonMS approved these changes Aug 7, 2019

View reviewed changes

pranavkm merged commit c24711c into release/3.0 Aug 7, 2019

ghost deleted the prkrishn/json-reader branch August 7, 2019 16:44

JunTaoLuo pushed a commit to dotnet/aspnetcore that referenced this pull request Feb 12, 2020

Use Utf8JsonReader in DotNetDispatcher (dotnet/extensions#2061)

86aff39

* Use Utf8JsonReader in DotNetDispatcher Fixes #10988 \n\nCommit migrated from dotnet/extensions@c24711c

JunTaoLuo pushed a commit to dotnet/aspnetcore that referenced this pull request Feb 15, 2020

Use Utf8JsonReader in DotNetDispatcher (dotnet/extensions#2061)

557bd8e

* Use Utf8JsonReader in DotNetDispatcher Fixes #10988 \n\nCommit migrated from dotnet/extensions@c24711c

ghost locked as resolved and limited conversation to collaborators May 27, 2023

	// Note that the JsonReader instance is intentionally not passed by ref (or asn in parameter) since we want a copy of the original reader.
	// Note that the JsonReader instance is intentionally not passed by ref (or an in parameter) since we want a copy of the original reader.

Use Utf8JsonReader in DotNetDispatcher #2061

Use Utf8JsonReader in DotNetDispatcher #2061

Conversation

pranavkm commented Jul 19, 2019

Uh oh!

pranavkm commented Jul 19, 2019

Uh oh!

BrennanConroy commented Jul 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BrennanConroy Jul 19, 2019 • 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!

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!

Choose a reason for hiding this comment

Uh oh!

SteveSandersonMS Jul 20, 2019 • 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!

Uh oh!

Uh oh!

javiercn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveSandersonMS Jul 20, 2019 • 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!

Uh oh!

SteveSandersonMS left a comment • 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!

pranavkm commented Aug 1, 2019

Uh oh!

BrennanConroy left a comment

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!

Choose a reason for hiding this comment

Uh oh!

pranavkm commented Aug 6, 2019

Uh oh!

BrennanConroy Jul 19, 2019 •

edited

Loading

SteveSandersonMS Jul 20, 2019 •

edited

Loading

SteveSandersonMS Jul 20, 2019 •

edited

Loading

SteveSandersonMS left a comment •

edited

Loading