Convert ComHost tasks to use System.Text.Json instead of Newtonsoft.Json #3087

jkoritzinsky · 2019-04-05T18:03:00Z

wli3 · 2019-04-05T20:52:34Z

eng/Versions.props

    <MicrosoftExtensionsDependencyModelVersion>2.1.0-preview2-26306-03</MicrosoftExtensionsDependencyModelVersion>
    <NETStandardLibraryNETFrameworkVersion>2.0.1-servicing-26011-01</NETStandardLibraryNETFrameworkVersion>
-    <NewtonsoftJsonVersion>9.0.1</NewtonsoftJsonVersion>
+    <MicrosoftBclJsonSourcesVersion>4.6.0-preview3.19128.7</MicrosoftBclJsonSourcesVersion>


Do you need this? it should be part of runtime already

ah we need that for full framework. @dsplaisted need your opinion, should we take system.text.json package dependency? And we should set up automatic insertion

wli3 · 2019-04-05T20:54:22Z

src/Tasks/Microsoft.NET.Build.Tasks/ArrayBufferWriter.cs

+    /// <summary>
+    /// Implementation of System.Buffers.IBufferWriter backed by ArrayPool.Shared.
+    /// </summary>
+    internal class ArrayBufferWriter : IBufferWriter<byte>, IDisposable


please add a comment like // TODO Remove once we have https://github.com/dotnet/corefx/issues/34894

Honestly, spreading this to every repo is getting silly. Let's wait for it to be in. I don't understand the rush to use this API. Particularly because we can't actually remove N.J until NuGet does.

wli3 · 2019-04-05T20:55:21Z

src/Tasks/Microsoft.NET.Build.Tasks/Microsoft.NET.Build.Tasks.csproj

    <PackageReference Include="Microsoft.Extensions.DependencyModel" Version="$(MicrosoftExtensionsDependencyModelVersion)" />
    <PackageReference Include="NuGet.ProjectModel" Version="$(NuGetProjectModelVersion)" />
    <PackageReference Include="NuGet.Build.Tasks.Pack" Version="$(NuGetBuildTasksPackVersion)" ExcludeAssets="All" />
+    <PackageReference Include="Microsoft.Bcl.Json.Sources" Version="$(MicrosoftBclJsonSourcesVersion)" GeneratePathProperty="true">


you should not need this

If we are going to use this package, we should only reference it for .NET Framework.

Also, why is this using a sources package instead of a library?

You would need to reference it for both of the TFMs here (net472, netcoreapp2.0). The library is built-in on netcoreapp3.0.

I think we can move to 3.0.

wli3

We should not need to reference the nuget package, let me check that, rest is good

wli3 · 2019-04-05T22:33:00Z

We need more discussion for adding system.text.json package. So i won't rush it to tonight's release 4 deadline

dsplaisted · 2019-04-05T22:48:30Z

src/Tasks/Microsoft.NET.Build.Tasks/Microsoft.NET.Build.Tasks.csproj

    <PackageReference Include="Microsoft.Extensions.DependencyModel" Version="$(MicrosoftExtensionsDependencyModelVersion)" />
    <PackageReference Include="NuGet.ProjectModel" Version="$(NuGetProjectModelVersion)" />
    <PackageReference Include="NuGet.Build.Tasks.Pack" Version="$(NuGetBuildTasksPackVersion)" ExcludeAssets="All" />
+    <PackageReference Include="Microsoft.Bcl.Json.Sources" Version="$(MicrosoftBclJsonSourcesVersion)" GeneratePathProperty="true">


If we are going to use this package, we should only reference it for .NET Framework.

Also, why is this using a sources package instead of a library?

jkoritzinsky · 2019-04-05T22:51:41Z

I don't know where the package is that isn't the sources package, so I grabbed the sources package. Where is the library package located?

ahsonkhan · 2019-04-06T01:36:01Z

src/Tasks/Microsoft.NET.Build.Tasks/ClsidMap.cs

                {
                    CustomAttributeValue<KnownType> data = attribute.DecodeValue(new TypeResolver());
-                    return (string)data.FixedArguments[0].Value;
+                    return $"{{{(string)data.FixedArguments[0].Value}}}";


I am curious, what's the reason for this change?

These GUIDs should be wrapped in curly brackets. It's something I noticed they weren't while I was working on this.

ahsonkhan · 2019-04-06T01:36:37Z

src/Tasks/Microsoft.NET.Build.Tasks/ClsidMap.cs

+
+            long prevBytesWritten = 0;
+
+            // Write some JSON, let's say an array of JSON objects in a loop


Comment doesn't match intent. Remove it?

ahsonkhan · 2019-04-06T01:36:59Z

src/Tasks/Microsoft.NET.Build.Tasks/ClsidMap.cs

+        {
+            const int SyncWriteThreshold = 1_000; // Copy to stream after writing this many bytes
+
+            ArrayBufferWriter buffer = new ArrayBufferWriter();


This is disposable. Add a using.

ahsonkhan · 2019-04-06T01:49:40Z

src/Tasks/Microsoft.NET.Build.Tasks/RegFreeComManifest.cs

-            string clsidMapText = File.ReadAllText(clsidMapPath);
-            using (StreamReader clsidMapReader = File.OpenText(clsidMapPath))
-            using (JsonTextReader jsonReader = new JsonTextReader(clsidMapReader))
+            JsonDocument clsidMap;


This needs to be disposed.

ahsonkhan · 2019-04-06T01:51:56Z

src/Tasks/Microsoft.NET.Build.Tasks/ClsidMap.cs


+        private static void WriteClsidMap(Stream clsidMapFile, Dictionary<string, ClsidEntry> map)
+        {
+            const int SyncWriteThreshold = 1_000; // Copy to stream after writing this many bytes


How large is the map expected to be? 1_000 was just an arbitrary example in the sample, and primarily for async writes.

If you are flushing the the stream synchronously anyway, I would remove this threshold all together (and flush after you have written everything) or make it much larger.

If you expect to go beyond 1 MB when writing JSON here (and definitely if you go beyond 2 GB), then keep the sync threshold but make it larger.

I find it quite troubling that we're expecting every consumer to think at this level. The BCL should provide a way to write Json to a Stream, IMHO.

Where does 1 MB guideline for synchronous case come from? How would we know to choose that?

(If we were writing a big payload)

The 1 MB guidelines is an implementation detail of using ArrayBufferWriter which is backed by the array pool. You can definitely continue to write up to 2 GB (and beyond until OOM), but beyond 1 MB, the array pool starts to allocate. That's why I suggested, that IF you were to set a sync threshold, 1 MB is reasonable. You don't have to, though, especially if allocations aren't a concern (writing to a growable stream without a sync threshold, for instance memory stream, has the same concern).

For performance oriented scenarios, we wanted to provide this fine grain control.

Let me rephrase. I wassaying "maybe I really want Serializer or DOM, but I am surprised that forward-only-writer-to-stream does not exist / is not directly supported".

JsonDocument is read-only today AFAICT from above. Are there plans to be able to build one up programatically and save like XDocument?

(I reach for XDocument a lot more than Xml serialization because I've been burned by serialization many, many times.)

I did think that there was no JsonSerializer avaialble yet though, happy to see that is in.

Are there plans to be able to build one up programatically and save like XDocument?

Not for 3.0, but something we are considering adding in the future based on feedback/user scenarios like yours.

FWIW, your concerns and feedback have been heard and hopefully addressed with the recent Utf8JsonWriter changes! We ended up making some trade-off between perf and usability to enable the scenarios you mentioned, @nguerrera :)

https://gist.github.com/ahsonkhan/f6f30656717548212693e5eaa49cece5
dotnet/corefx#36961

Presumably, we should re-visit this PR given the new changes!

Sweet! Thanks!

ahsonkhan · 2019-04-06T02:09:10Z

src/Tasks/Microsoft.NET.Build.Tasks/ClsidMap.cs

+                if (prevBytesWritten > SyncWriteThreshold)
+                {
+                    json.Flush(isFinalBlock: false);
+                    buffer.CopyTo(clsidMapFile);


I believe there is bug in the sample which will cause issues here.

After flushing, you need to reset the json writer local by re-instantiating it.

If you are keeping this SyncWriteThreshold pattern here (which in general should not be needed), change it to the following:

foreach (KeyValuePair<string, ClsidEntry> clsid in map) { json.WriteStartObject(clsid.Key); json.WriteString("assembly", clsid.Value.Assembly); json.WriteString("type", clsid.Value.Type); json.WriteEndObject(); if (json.BytesWritten > SyncWriteThreshold) { json.Flush(isFinalBlock: false); buffer.CopyTo(clsidMapFile); json = new Utf8JsonWriter(buffer, json.GetCurrentState()); } }

I'll fix the sample docs and call out that this pattern is mainly useful for async writes, not synchronous ones. Thanks!

Btw, based on your porting effort, feedback/contribution to the porting guide would be most welcome so it can be improved :)

https://github.com/dotnet/corefx/blob/master/src/System.Text.Json/porting_guide/README.md

nguerrera · 2019-04-06T02:29:56Z

src/Tasks/Microsoft.NET.Build.Tasks/Microsoft.NET.Build.Tasks.csproj

+    <PackageReference Include="Microsoft.Bcl.Json.Sources" Version="$(MicrosoftBclJsonSourcesVersion)" GeneratePathProperty="true">
+      <PrivateAssets>All</PrivateAssets>
+    </PackageReference>
+    <PackageReference Include="System.Memory" Version="4.5.2" />


I'm not super kean on more copy-local assemblies for the desktop build. We ran into issues with System.Memory and S.RC.Unsafe and mismatched versions in PresentationBuildTasks.dll. I don't know if it's worth adding more dependencies if we can't remove NewtonSoft.Json. And we can't because NuGet depends on it.

I think I'd wait for our dependencies to stop using newtonsoft.json. This is going to just create more DLLs to ship and load, not fewer.

I think we can move to 3.0.

The decision to port is up to you, but for clarity, if this moves to 3.0 and the built-in library is being used, then there won't be extra copy-local assemblies like System.Memory.

It can't be 3.0 only. It can be 3.0 + 472. We have to run in Framework for VS msbuild

nguerrera

Thanks for doing this, but I don't think we should take this yet. I see more dlls deployed and loaded, plus a bunch of copied code that comes with TODO debt. I don't see any actual value for now. At some point, the value will come but we're not there yet.

jkoritzinsky · 2019-04-08T17:55:45Z

I'm going to close this for now since we aren't going to merge this in for quite a bit. I'll re-open it when we're closer to taking it.

jkoritzinsky · 2019-04-19T16:15:05Z

@ahsonkhan is there a way to serialize a System.Collections.Generic.Dictionary<TKey, TValue> with the JsonSerializer APIs?

jkoritzinsky · 2019-04-19T16:23:58Z

Also @nguerrera @ahsonkhan the newest drop of Microsoft.Bcl.Json.Sources uses the ValueTask<T> type, which requires a reference to System.Threading.Tasks.Extensions on .NET Framework. I know there's been a lot of talk about dependency problems on Framework so I don't want to open a new PR until we've decided what to do.

nguerrera · 2019-04-19T16:25:36Z

Don't open a new PR yet. We need to think about the dependencies more carefully. `That isn't resolved yet. I'm happier with the new API shape, but that is orthogonal to the dependency issues.

nguerrera · 2019-04-19T16:25:50Z

Maybe we should meet up?

jkoritzinsky · 2019-04-19T16:27:39Z

Sounds good to me.

FYI I'm working remotely/oof until Tuesday.

nguerrera · 2019-04-19T16:28:37Z

(And I'm out of office all of next week, and booked all day today.)

wli3 · 2019-04-19T22:48:52Z

@jkoritzinsky @nguerrera loop me in

ahsonkhan · 2019-04-19T22:52:32Z

is there a way to serialize a System.Collections.Generic.Dictionary<TKey, TValue> with the JsonSerializer APIs?

Not currently. It's WIP. See dotnet/corefx#37033 for concrete TKey, TValue. The polymorphic support is still pending.

Maybe we should meet up?
FYI I'm working remotely/oof until Tuesday.
(And I'm out of office all of next week, and booked all day today.)
loop me in

Sure. Please setup a meeting sometime the week of April 29 or post Build :)
Let's figure it out!

cc @terrajobst

…build 20191004.12 (dotnet#3087) - Microsoft.NET.Sdk.Razor - 3.1.0-preview1.19504.12

jkoritzinsky added 3 commits April 4, 2019 16:55

Add reference to Microsoft.Bcl.Json.Sources source package.

f7f53dc

Convert reg-free com manifest generation to System.Text.Json.

ed58b77

Move ComHost tasks over to System.Text.Json.

b781924

wli3 reviewed Apr 5, 2019

View reviewed changes

wli3 suggested changes Apr 5, 2019

View reviewed changes

Add TODO remove comment.

1f5ef15

dsplaisted approved these changes Apr 5, 2019

View reviewed changes

ahsonkhan reviewed Apr 6, 2019

View reviewed changes

nguerrera reviewed Apr 6, 2019

View reviewed changes

nguerrera suggested changes Apr 6, 2019

View reviewed changes

jkoritzinsky closed this Apr 8, 2019

dsplaisted pushed a commit to dsplaisted/sdk that referenced this pull request Feb 19, 2020

Update dependencies from https://github.com/aspnet/AspNetCore-Tooling …

b576959

…build 20191004.12 (dotnet#3087) - Microsoft.NET.Sdk.Razor - 3.1.0-preview1.19504.12


		long prevBytesWritten = 0;

		// Write some JSON, let's say an array of JSON objects in a loop

Convert ComHost tasks to use System.Text.Json instead of Newtonsoft.Json #3087

Convert ComHost tasks to use System.Text.Json instead of Newtonsoft.Json #3087

Uh oh!

Conversation

jkoritzinsky commented Apr 5, 2019

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!

wli3 left a comment

Choose a reason for hiding this comment

Uh oh!

wli3 commented Apr 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkoritzinsky commented Apr 5, 2019

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!

ahsonkhan Apr 6, 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!

ahsonkhan Apr 9, 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!

nguerrera Apr 9, 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!

ahsonkhan Apr 6, 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!

ahsonkhan Apr 6, 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

ahsonkhan Apr 6, 2019 •

edited

Loading

ahsonkhan Apr 9, 2019 •

edited

Loading

nguerrera Apr 9, 2019 •

edited

Loading

ahsonkhan Apr 6, 2019 •

edited

Loading

ahsonkhan Apr 6, 2019 •

edited

Loading

nguerrera left a comment •

edited

Loading