Remove syntax serialization logic. #70277

CyrusNajmabadi · 2023-10-06T18:57:36Z

Reviewed and approved by API review. We have shipped already with this functionality deprecated. This PR now removes the functionality from our next release.

--

The linked issue explains the motivation here. But the TLDR is: this functionality only existed (and was only usable) for exactly one scenario. A single host (like VS) dumping trees out of memory to disk, and then loading them back into memory in the same session. The data dumped to disk contains references to live memory objects, and as such could not work across multiple processes (like between VS and OOP), or even within the same host across different sessions (like if you closed/reopened VS). We only had this for the 32bit days when memory was scarce, and dumping trees to disk allowed us to free up the 32bit address space. Those days are long behind us, and we've already removed the codepaths that actually dump trees to disk. So nothing in roslyn itself is even utilizing this functionality anymore.

This saves 20k of code, and allows us to now make interesting changes to the serialization subsystem that will benefit our current architecture. For example, moving it to sit on pipes, instead of streams, or allowing it to be replaced with something like messagepack, instead of our home-rolled serialization logic (which was only necesary to do wacky stuff like encode pointers to live memory objects stored in static locations).

CyrusNajmabadi · 2023-10-27T19:43:54Z

src/Analyzers/Core/Analyzers/Helpers/DiagnosticHelper.cs

@@ -276,14 +276,11 @@ static string EncodeIndices(IEnumerable<int> indices, int additionalLocationsLen
            return $"https://learn.microsoft.com/dotnet/fundamentals/code-analysis/style-rules/{id.ToLowerInvariant()}";
        }

-        public sealed class LocalizableStringWithArguments : LocalizableString, IObjectWritable
+        public sealed class LocalizableStringWithArguments : LocalizableString


IObjectWritable as a concept goes away. Instead of the serialization system taking in arbitrary objects, querying for that, and having that object then decide what to do, serialization is owned entirely on the outside by the objects, who just use our serialization system as a dumb stream wrapper to do things like write ints, bools, and strings to. Only basic constructs are supported (like primitive types, and arrays of types). Arbitrary object graphs are now handled by the caller (note: we have no such cases in roslyn anyways. it was only 'syntax' with its heterogenous graph structure).

LocalizableStirng used to have to be serializable, because it was stored in diagnostics as the messages, and diagnostics were stored inside a tree for syntax errors, and as such needed to be serializable if we serialized the tree. Now that we no longer serialize trees, neither diagnostics nor locstrings need this functionality.

CyrusNajmabadi · 2023-10-27T19:44:56Z

src/Analyzers/Core/Analyzers/Helpers/DiagnosticHelper.cs

        {
            private readonly LocalizableString _messageFormat;
            private readonly string[] _formatArguments;

-            static LocalizableStringWithArguments()
-                => ObjectBinder.RegisterTypeReader(typeof(LocalizableStringWithArguments), reader => new LocalizableStringWithArguments(reader));


ObjectBinder.RegisterTypeReader was how types specified how they could be deseriealized. But it worked by registering a callback function at an index in an array, then emitting that array index into the serialized stream of bytes. this, of course, coudl not work across processes or across sessions as that index woudl never be the same. this is why this functionality is so practically useless.

CyrusNajmabadi · 2023-10-27T19:46:09Z

src/Compilers/CSharp/Portable/Errors/MessageProvider.cs

@@ -14,26 +14,14 @@

 namespace Microsoft.CodeAnalysis.CSharp
 {
-    internal sealed class MessageProvider : CommonMessageProvider, IObjectWritable
+    internal sealed class MessageProvider : CommonMessageProvider


MessageProvider is part of loc strings. Also doesn't need serialization now.

CyrusNajmabadi · 2023-10-27T19:46:26Z

src/Compilers/CSharp/Portable/Errors/SyntaxDiagnosticInfo.cs

@@ -10,11 +10,6 @@ namespace Microsoft.CodeAnalysis.CSharp
 {
    internal class SyntaxDiagnosticInfo : DiagnosticInfo


diagnostic-infos are part of diagnostics. which no longer need to be serialized now.

CyrusNajmabadi · 2023-10-27T19:46:58Z

src/Compilers/CSharp/Portable/Errors/XmlSyntaxDiagnosticInfo.cs

-        }
-
-        #endregion
-
        public override string GetMessage(IFormatProvider? formatProvider = null)
        {
            var culture = formatProvider as CultureInfo;


writing this comment here above the generated file. THe generated file is where the buld of the deletion comes from as we no longer need to emit all that goop for syntax serialization.

CyrusNajmabadi · 2023-10-27T19:48:15Z

src/Compilers/CSharp/Portable/Syntax/InternalSyntax/CSharpSyntaxNode.cs

@@ -53,11 +53,6 @@ internal CSharpSyntaxNode(SyntaxKind kind, DiagnosticInfo[] diagnostics, SyntaxA
            GreenStats.NoteGreen(this);
        }

-        internal CSharpSyntaxNode(ObjectReader reader)


a lot of the core syntax types that follow will just see their internal serialization support removed.

CyrusNajmabadi · 2023-10-27T19:49:44Z

src/Compilers/Core/CodeAnalysisTest/ObjectSerializationTests.cs

-            new PrimitiveArrayMemberTest();
-            new PrimitiveMemberTest();
-            new PrimitiveValueTest();
-        }


i deleted all the tests related to object graph serialization. all the tests testing things like writing out primitives/arrays/strings/etc. all remain and are still relevant.

CyrusNajmabadi · 2023-10-27T19:50:29Z

src/Compilers/Core/Portable/Serialization/ObjectBinder.cs

-        /// <summary>
-        /// Map from a <see cref="Type"/> to the corresponding index in <see cref="s_types"/> and
-        /// <see cref="s_typeReaders"/>.  <see cref="ObjectWriter"/> will write out the index into
-        /// the stream, and <see cref="ObjectReader"/> will use that index to get the reader used


this was the part that was truly nasty.

CyrusNajmabadi · 2023-10-27T19:51:17Z

src/Compilers/Core/Portable/Serialization/ObjectReader.cs

-                }
-
-                return array;
+                throw ExceptionUtilities.UnexpectedValue(elementKind);


we don't support arrays of aribtrary objects. it always needs to be an array of some primitive type (which is all we ever do in roslyn).

jcouv

Done with review pass (iteration 18). Only looked at Compiler and Tools folders

CyrusNajmabadi · 2023-11-06T21:08:51Z

PR feedback responded to.

…ists only in the IDE layer

jcouv

LGTM Thanks (iteration 26). Looked at Compilers and Tools changes only

CyrusNajmabadi · 2023-11-06T21:31:57Z

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/CompilerExtensions.projitems

@@ -266,6 +259,9 @@
    <Compile Include="$(MSBuildThisFileDirectory)Options\IPublicOption.cs" />
    <Compile Include="$(MSBuildThisFileDirectory)Options\PublicOptionFactory.cs" />
    <Compile Include="$(MSBuildThisFileDirectory)Progress\NullProgress.cs" />
+    <Compile Include="$(MSBuildThisFileDirectory)Serialization\ObjectReader.cs" />
+    <Compile Include="$(MSBuildThisFileDirectory)Serialization\ObjectWriter.cs" />
+    <Compile Include="$(MSBuildThisFileDirectory)Serialization\TextEncodingKind.cs" />


these types moved out of hte compielr entirely. it's now just pulled into the IDE layers and nothing else.

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Serialization/ObjectWriter.cs

CyrusNajmabadi · 2023-11-06T21:37:38Z

@RikkiGibson @333fred @jjonescz for another set of eyes when available.

@ToddGrun for IDE side. Thanks!

…alization/ObjectWriter.cs

…i/roslyn into nodeSerialization2

jaredpar

Signing off on compiler side

jaredpar · 2023-11-06T23:54:53Z

src/Compilers/Test/Utilities/CSharp/CSharpTestSource.cs

@@ -39,23 +39,9 @@ private CSharpTestSource(object value)
        {
            var stringText = SourceText.From(text, encoding ?? Encoding.UTF8, checksumAlgorithm);
            var tree = SyntaxFactory.ParseSyntaxTree(stringText, options ?? TestOptions.RegularPreview, path);


Could simplify this to just return SyntaxFactory...

jaredpar · 2023-11-06T23:55:19Z

src/Compilers/Test/Utilities/VisualBasic/BasicTestSource.vb

@@ -23,23 +23,9 @@ Public Structure BasicTestSource

        Dim sourceTest = SourceText.From(text, If(encoding, Encoding.UTF8), checksumAlgorithm)
        Dim tree = SyntaxFactory.ParseSyntaxTree(sourceTest, If(options, TestOptions.RegularLatest), path)


Could simplify this to Return SyntaxFactory...

ToddGrun · 2023-11-07T17:35:42Z

src/Compilers/Core/Portable/Serialization/ObjectWriter.cs

-        private static Exception NoSerializationWriterException(string typeName)
-        {
-            return new InvalidOperationException(string.Format(Resources.Cannot_serialize_type_0, typeName));
-        }


}

I was going to make a comment that the comment below is no longer in compiler and there is a bidirectional map available. But then I noticed that it uses ImmutableDictionarys for it's mapping, which is definitely not perf friendly. As the usage here doesn't look like it needs modifications, would it make sense for there to be a NonMutableBidirectionalMap?

yes. that would make a ton of sense.

ToddGrun

CyrusNajmabadi added 7 commits October 6, 2023 10:10

Remove code for node serialization

cb0102b

Update generators

6fb6240

Fix

8839f7f

Remove recursion from object serialization

06a1674

Remove more

bbbb772

in progress

377e4cf

Remove IObjectWritable

8c3fe33

dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Oct 6, 2023

CyrusNajmabadi mentioned this pull request Oct 6, 2023

Deprecate syntax serialization logic. #70275

Closed

CyrusNajmabadi added 7 commits October 6, 2023 12:26

Remove tests

ab73f4d

Remove tests

265799a

Merge remote-tracking branch 'upstream/main' into nodeSerialization2

77df906

Merge remote-tracking branch 'upstream/main' into nodeSerialization2

9dcb386

fix test

7f9c2b9

Merge branch 'DisableTests' into nodeSerialization2

8375ba6

Merge remote-tracking branch 'upstream/main' into nodeSerialization2

b76ae85

CyrusNajmabadi changed the title ~~WIP - Remove IObjectWritable~~ Remove syntax serialization logic. Oct 27, 2023

CyrusNajmabadi marked this pull request as ready for review October 27, 2023 19:41

CyrusNajmabadi requested review from a team as code owners October 27, 2023 19:41

CyrusNajmabadi commented Oct 27, 2023

View reviewed changes

jcouv reviewed Nov 6, 2023

View reviewed changes

CyrusNajmabadi added 2 commits November 6, 2023 13:06

Update breaking changes docs

256e4d4

Remove recursion depth checks

e095de0

CyrusNajmabadi requested a review from jcouv November 6, 2023 21:09

CyrusNajmabadi added 4 commits November 6, 2023 13:16

Remove unused type

a325c49

remove unneeded logic

05638e1

Move serialization code entirely out of hte compiler layer. it now ex…

3aea819

…ists only in the IDE layer

Simplify

9f05fa7

jcouv approved these changes Nov 6, 2023

View reviewed changes

jcouv self-assigned this Nov 6, 2023

CyrusNajmabadi commented Nov 6, 2023

View reviewed changes

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Serialization/ObjectWriter.cs Outdated Show resolved Hide resolved

Simplify

a7af896

CyrusNajmabadi added 4 commits November 6, 2023 13:37

Update src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Seri…

71e36d1

…alization/ObjectWriter.cs

Lint

fb5acb1

Merge branch 'nodeSerialization2' of https://github.com/CyrusNajmabad…

137d60d

…i/roslyn into nodeSerialization2

Lint

59dd654

jaredpar self-assigned this Nov 6, 2023

jaredpar approved these changes Nov 6, 2023

View reviewed changes

ToddGrun reviewed Nov 7, 2023

View reviewed changes

ToddGrun approved these changes Nov 7, 2023

View reviewed changes

CyrusNajmabadi merged commit 545c540 into dotnet:main Nov 7, 2023
27 checks passed

CyrusNajmabadi deleted the nodeSerialization2 branch November 7, 2023 17:38

ghost added this to the Next milestone Nov 7, 2023

allisonchou mentioned this pull request Nov 9, 2023

[Automated] PRs inserted in VS build main-34309.104 #70754

Closed

RikkiGibson modified the milestones: Next, 17.9 P2 Nov 28, 2023

aetos382 mentioned this pull request Jan 15, 2024

CSharpSyntaxNode.DeserializeFrom(stream) does not work #8676

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove syntax serialization logic. #70277

Remove syntax serialization logic. #70277

CyrusNajmabadi commented Oct 6, 2023 •

edited

CyrusNajmabadi Oct 27, 2023

CyrusNajmabadi Oct 27, 2023

CyrusNajmabadi Oct 27, 2023

CyrusNajmabadi Oct 27, 2023

CyrusNajmabadi Oct 27, 2023

CyrusNajmabadi Oct 27, 2023

CyrusNajmabadi Oct 27, 2023

CyrusNajmabadi Oct 27, 2023

CyrusNajmabadi Oct 27, 2023

CyrusNajmabadi Oct 27, 2023 •

edited

jcouv left a comment

CyrusNajmabadi commented Nov 6, 2023

jcouv left a comment

CyrusNajmabadi Nov 6, 2023

CyrusNajmabadi commented Nov 6, 2023

jaredpar left a comment

jaredpar Nov 6, 2023

jaredpar Nov 6, 2023

ToddGrun Nov 7, 2023

CyrusNajmabadi Nov 7, 2023

ToddGrun left a comment

		@@ -10,11 +10,6 @@ namespace Microsoft.CodeAnalysis.CSharp
		{
		internal class SyntaxDiagnosticInfo : DiagnosticInfo

		@@ -23,23 +23,9 @@ Public Structure BasicTestSource

		Dim sourceTest = SourceText.From(text, If(encoding, Encoding.UTF8), checksumAlgorithm)
		Dim tree = SyntaxFactory.ParseSyntaxTree(sourceTest, If(options, TestOptions.RegularLatest), path)

Remove syntax serialization logic. #70277

Remove syntax serialization logic. #70277

Conversation

CyrusNajmabadi commented Oct 6, 2023 • edited

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

CyrusNajmabadi Oct 27, 2023 • edited

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Nov 6, 2023

jcouv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Nov 6, 2023

jaredpar 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

Choose a reason for hiding this comment

ToddGrun left a comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Oct 6, 2023 •

edited

CyrusNajmabadi Oct 27, 2023 •

edited