Switch to json serialization without adding support for precomputed caches #5983

Forgind · 2020-12-21T17:38:12Z

We need to move off BinaryFormatter.

Test of basic functionality (creates simple cache in json and reads it) passed.

This gives RAR the ability to save off a modified state file and load it for later use. Still to do: 1) Verify that the precomputed cache read in is real. 2) Change logging such that it will log messages instead of errors or warnings if there are problems with deserializing the precomputed cache 3) Comments at relevant points 4) Switch serialization mode to another form (json?) 5) Validation + performance tests 6) Have SDK opt in

It was previously serializable via BinaryFormatter. This makes it serializable via System.Text.Json's JsonConverter.

Add support for serializing SystemState using System.Text.Json rather than the older BinaryFormatter.

… just when serializing and deserializing the precomputed cache

I have not yet measured how long it takes to read a MVID vs. reading and parsing its references, nor have I made unit tests.

…ache

...although they don't work yet

…son-serialization

The version of Arcade MSBuild currently uses uses Roslyn version 3.3.1. This updates that, but it should be removed when we've updated to a more modern version of arcade.

…into json-serialization

…son-serialization

… into serialization-only

rainersigwald · 2020-12-21T21:46:01Z

eng/Packages.props

+  <PropertyGroup>
+      <NuGetPackageVersion>5.9.0-preview.1.6870</NuGetPackageVersion>
+      <NuGetBuildTasksVersion Condition="'$(NuGetBuildTasksVersion)' == ''">$(NuGetPackageVersion)</NuGetBuildTasksVersion>
+      <NuGetCommandsVersion Condition="'$(NuGetCommandsVersion)' == ''">$(NuGetPackageVersion)</NuGetCommandsVersion>
+      <NuGetProtocolVersion Condition="'$(NuGetProtocolVersion)' == ''">$(NuGetPackageVersion)</NuGetProtocolVersion>
+  </PropertyGroup>


This looks like a bad merge that's overwriting NuGet updates.

rainersigwald · 2020-12-21T21:47:41Z

src/Tasks/Microsoft.Build.Tasks.csproj

@@ -986,13 +986,15 @@
  </ItemGroup>
  <ItemGroup>
    <PackageReference Include="System.Collections.Immutable" />
+    <PackageReference Include="System.Reflection.Metadata" />


We're using this in full framework now?

Among the things that (presumably) was useful at some stage in serialization, though possibly just for BinaryFormatter-serialized caches.

rainersigwald · 2020-12-21T21:49:23Z

src/Tasks/Microsoft.Build.Tasks.csproj

    <PackageReference Include="System.Resources.Extensions" />
  </ItemGroup>

  <!-- Tasks need to mimic redistributing the compilers, so add references to both full framework and .net core -->
  <ItemGroup>
    <!-- Reference this package to get binaries at runtime even when Arcade is not adding compiler references -->
    <PackageReference Include="Microsoft.Net.Compilers.Toolset" ExcludeAssets="all" Condition="'$(UsingToolMicrosoftNetCompilers)' == 'false'" />
+    <PackageReference Include="System.Text.Json" />


If this is a normal reference it should go in the PackageReference group above. The comments don't apply to normal references.

rainersigwald · 2020-12-22T15:29:57Z

src/Tasks/AssemblyDependency/ResolveAssemblyReference.cs

        {
            if (!string.IsNullOrEmpty(_stateFile) && _cache.IsDirty)
            {
-                _cache.SerializeCache(_stateFile, Log);
+                var deserializeOptions = new JsonSerializerOptions() { Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping };


So . . . the Unsafe part of this name sure stands out to me. Is it necessary? Is it safe? If so, comment an explanation of why.

I think so, and I think so. If I remember correctly, this has to do with whether characters are escaped, and this specifies they don't need escaping, but almost everything in the cache is ultimately a number, a string, a bool, or a version, so other random characters should be flagged as "wrong type" outside the string, which should accept them without running them as code or immediately reject them depending on the circumstance. I don't remember which characters were being serialized incorrectly, but something was.

I still want to know this for sure, and with comments in the source explaining.

rainersigwald · 2020-12-22T15:31:01Z

src/Tasks/AssemblyDependency/ResolveAssemblyReference.cs

-                _cache.SerializeCache(_stateFile, Log);
+                var deserializeOptions = new JsonSerializerOptions() { Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping };
+                deserializeOptions.Converters.Add(new SystemState.Converter());
+                File.WriteAllText(_stateFile, JsonSerializer.Serialize<SystemState>(_cache, deserializeOptions));


Can you call the overload that takes a stream so we don't have to materialize the possibly-LOH full string?

Unfortunately, no. Not available in .NET Framework. If you want to switch to .net 5...

Not available in .NET Framework.

What specifically isn't? I see

public static ValueTask<TValue?> DeserializeAsync<TValue>(Stream utf8Json, JsonSerializerOptions? options = null, CancellationToken cancellationToken = default(CancellationToken))

in the net461 decompilation.

If you want to switch to .net 5

We are doing so in #5515.

Looks like it's really just that the documentation doesn't accurately reflect the frameworks it's available for:

rainersigwald · 2020-12-22T15:42:37Z

src/Tasks/StateFileBase.cs

@@ -64,9 +64,10 @@ internal virtual void SerializeCache(string stateFile, TaskLoggingHelper log)
        /// <summary>
        /// Reads the specified file from disk into a StateFileBase derived object.
        /// </summary>
-        internal static StateFileBase DeserializeCache(string stateFile, TaskLoggingHelper log, Type requiredReturnType)
+        internal static T DeserializeCache<T>(string stateFile, TaskLoggingHelper log, bool logWarnings = true) where T : StateFileBase


The changes in this method look good but I don't understand why they're being made. Aren't we getting rid of BinaryFormatter?

Eventually, yes, but StateFileBase is used for other classes, too. I did some cleanup while I was looking at it.

src/Shared/AssemblyNameExtension.cs

rainersigwald · 2020-12-22T15:54:26Z

src/Tasks/SystemState.cs

-        /// Construct.
-        /// </summary>
-        internal SystemState()
+        internal sealed class Converter : JsonConverter<SystemState>


Can you go into a fair amount of detail on why this fairly-custom converter is here? Is it required for functionality? Is it a perf improvement? Will it be necessary after dotnet/runtime#1568 lands? Why choose the explicit-converter approach over the JsonConverter attribute approach?

Note to self: haven't looked at this part because waiting on 👆🏻

I started out with a not-customized implementation, had some difficulty getting everything to be serialized, and asked for help. You told me to write a customer serializer. For the first part of that, see 8b3b613.

I think you suggested this is more secure, serializes everything, and hopefully runs faster than using a default converter.

rainersigwald · 2020-12-22T15:57:26Z

src/Tasks/SystemState.cs

        /// <summary>
        /// Flag that indicates
        /// </summary>
        /// <value></value>
        internal bool IsDirty
        {
            get { return isDirty; }
+            set { isDirty = value; }


Adding a setter here feels wrong. Generally dirtiness is a result of mutating something and not directly manipulable. Is that not true here?

It is true, but this is very useful in the test I deleted. (It's among the "not actually necessary but present" changes I mentioned.) I'd prefer not to bother with deleting and readding them.

rainersigwald · 2020-12-22T15:59:09Z

src/Tasks/SystemState.cs

@@ -540,7 +669,7 @@ out fileState.frameworkName

            dependencies = fileState.dependencies;
            scatterFiles = fileState.scatterFiles;
-            frameworkName = fileState.frameworkName;
+            frameworkName = fileState.FrameworkNameAttribute;


Why go through the getter when we have direct access?

Does it matter? The getter just returns it.

I think I tried to eliminate the field but found it was necessary and put it back.

Please put it all the way back. Unnecessary churn like this makes reviews harder and generally hurts understandability of the codebase.

rainersigwald · 2021-01-07T23:13:32Z

src/Tasks/AssemblyDependency/ResolveAssemblyReference.cs

        {
            if (!string.IsNullOrEmpty(_stateFile) && _cache.IsDirty)
            {
-                _cache.SerializeCache(_stateFile, Log);
+                var deserializeOptions = new JsonSerializerOptions() { Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping };


I still want to know this for sure, and with comments in the source explaining.

benvillalobos

Partial review

benvillalobos · 2021-01-08T00:51:17Z

src/Tasks/StateFileBase.cs

                        {
-                            log.LogMessageFromResources("General.CouldNotReadStateFileMessage", stateFile, log.FormatResourceString("General.IncompatibleStateFileType"));
-                            retVal = null;
+                            log.LogMessageFromResources("General.CouldNotReadStateFile", stateFile, log.FormatResourceString("General.IncompatibleStateFileType"));


Isn't this a slightly different codepath now? The older version checked retVal._serializedVersion != CurrentSerializationVersion before logging this, should we check the same here?

I don't see what you mean? It looks to me like the retVal._serializedVersion != CurrentSerializationVersion check is after logging a warning (always) in the old code. The changes here are:

The conditions were mutually exclusive --> switch to else if

Make function parameterized instead of explicitly taking a type as an argument

Give the option of not logging warnings

benvillalobos · 2021-01-08T01:01:05Z

src/Shared/AssemblyNameExtension.cs

+                        throw new JsonException();
+                    }
+                    string parameter = reader.GetString();
+                    reader.Read();


What is this capturing exactly? Could you comment the first use of reader.Read() in this context?

This probably does deserve a comment, since it's kinda overloaded.

The Json reader is set up such that every thing (technical term) of interest has its own type. In particular, when specifying that a particular property has a particular value, it has the "PropertyName" token followed by the token(s) representing the property. If that property is not null, it will start with a "StartObject" token if it's an object or a "StartArray" token if it's an array, or just have a value we read below. If it's null, it just starts with null, and there isn't anything else.

In this case, I read the first token. If it's null, that means that the json has something like "propertyName"=null, which is valid though avoided unless you modify it by hand. Having the null check you commented on means that we don't keep reading the object if it happens to be null. GetString() would have returned a valid property name on the ANE that wasn't set.

If the property isn't null, it skips over the "StartObject", etc. and lets us start reading the interesting stuff immediately or parses the token as appropriate. Does that make sense?

benvillalobos · 2021-01-08T01:03:50Z

src/Shared/AssemblyNameExtension.cs

+                    }
+                    string parameter = reader.GetString();
+                    reader.Read();
+                    if (reader.TokenType == JsonTokenType.Null)


What does it mean when we see null here? What would we have just read from GetString() if we fall into this case?

Forgind added 30 commits July 14, 2020 16:03

PR comments pt. 1

1d73559

Resolve first three bullet points

036c5a3

Next steps

8b3b613

revert to BinaryFormatter

6160d48

Serialize mvid

9b95494

Design change

43195f4

Serialize AssemblyNameExtension

9045d3d

It was previously serializable via BinaryFormatter. This makes it serializable via System.Text.Json's JsonConverter.

Serialize SystemState

ecf24de

Add support for serializing SystemState using System.Text.Json rather than the older BinaryFormatter.

Opt into new serialization behavior for all serialization rather than…

b25d160

… just when serializing and deserializing the precomputed cache

Merge branch 'master' into json-serialization

9fa92fb

PR comments

5608066

I have not yet measured how long it takes to read a MVID vs. reading and parsing its references, nor have I made unit tests.

Make DeserializeCache take a generic StateFileBase

ccb508d

Use FileSystems.Default.FileExists

354de87

Convert instanceLocalFileStateCache to a dictionary

fd76e0b

Fix generic

b1a7695

Merge branch 'master' of https://github.com/dotnet/msbuild into rar-c…

d71f2a6

…ache

Build

fbe1028

Fix bad changes

d790150

Added tests 1

a759006

Finish tests

767f1cc

...although they don't work yet

Create delegates to mock system calls

412c21b

Merge branch 'rar-cache' of https://github.com/forgind/msbuild into j…

0450dd2

…son-serialization

Small fixups

9f930f4

Make constructor internal

58217f6

PR feedback

2043c3f

Avoid using overly new feature

853fd38

Real ANE

bda6b37

Merge https://github.com/dotnet/msbuild into rar-cache

a2c0f42

Merge branch 'rar-cache' of https://github.com/forgind/msbuild into j…

d92e5da

…son-serialization

Forgind added 7 commits November 12, 2020 09:10

fixed test

37a7583

Update versions

a557722

The version of Arcade MSBuild currently uses uses Roslyn version 3.3.1. This updates that, but it should be removed when we've updated to a more modern version of arcade.

Merge branch 'update-versions' of https://github.com/forgind/msbuild …

5a6851f

…into json-serialization

Remove MVID check

59aaab1

Merge branch 'rar-cache' of https://github.com/forgind/msbuild into j…

df2d5c9

…son-serialization

Merge branch 'json-serialization' of https://github.com/forgind/msbuild…

b2f7507

… into serialization-only

Remove precomputed cache

11978bb

Forgind marked this pull request as draft December 21, 2020 18:24

Add fallback cache creation

fcbeb81

Forgind force-pushed the serialization-only branch from c45c079 to fcbeb81 Compare December 21, 2020 18:30

Forgind marked this pull request as ready for review December 21, 2020 19:31

rainersigwald requested changes Dec 22, 2020

View reviewed changes

Forgind added 3 commits December 22, 2020 10:00

PR comments

2f3ed8d

Switch to async

08aede6

Make tests pass

01a3b56

rainersigwald requested changes Jan 7, 2021

View reviewed changes

benvillalobos reviewed Jan 8, 2021

View reviewed changes

Forgind added 2 commits January 8, 2021 07:41

Use frameworkName

63f4b00

Add comment

d50dbb4

Forgind closed this Jan 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to json serialization without adding support for precomputed caches #5983

Switch to json serialization without adding support for precomputed caches #5983

Forgind commented Dec 21, 2020 •

edited

Loading

rainersigwald Dec 21, 2020

rainersigwald Dec 21, 2020

Forgind Dec 22, 2020

rainersigwald Dec 21, 2020

rainersigwald Dec 22, 2020

Forgind Dec 22, 2020

rainersigwald Jan 7, 2021

rainersigwald Dec 22, 2020

Forgind Dec 22, 2020

rainersigwald Dec 22, 2020

Forgind Dec 22, 2020 •

edited

Loading

rainersigwald Dec 22, 2020

Forgind Dec 22, 2020

rainersigwald Dec 22, 2020

rainersigwald Dec 22, 2020

Forgind Dec 22, 2020

rainersigwald Dec 22, 2020

Forgind Dec 22, 2020

rainersigwald Dec 22, 2020

Forgind Dec 22, 2020

rainersigwald Jan 7, 2021

rainersigwald Jan 7, 2021

benvillalobos left a comment

benvillalobos Jan 8, 2021

Forgind Jan 8, 2021

benvillalobos Jan 8, 2021

Forgind Jan 8, 2021 •

edited

Loading

benvillalobos Jan 8, 2021

Switch to json serialization without adding support for precomputed caches #5983

Switch to json serialization without adding support for precomputed caches #5983

Conversation

Forgind commented Dec 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Forgind Dec 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benvillalobos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Forgind Jan 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Forgind commented Dec 21, 2020 •

edited

Loading

Forgind Dec 22, 2020 •

edited

Loading

Forgind Jan 8, 2021 •

edited

Loading