Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate syntax serialization logic. #70275

Closed
CyrusNajmabadi opened this issue Oct 6, 2023 · 2 comments · Fixed by #70277
Closed

Deprecate syntax serialization logic. #70275

CyrusNajmabadi opened this issue Oct 6, 2023 · 2 comments · Fixed by #70277
Assignees
Labels
api-approved API was approved in API review, it can be implemented Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature Request
Milestone

Comments

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Oct 6, 2023

Background and Motivation

The roslyn API contains a mechanism to serialize a syntax node to a stream, and then read it back again.

The methods are:

public class SyntaxNode
{
    public virtual void SerializeTo(Stream stream, CancellationToken cancellationToken = default)
}

public class CSharpSyntaxNode
{
   public static SyntaxNode DeserializeFrom(Stream stream, CancellationToken cancellationToken = default)
}

// And the VB equivalent.

These APIs were created for the IDE back in the 32bit days so that teh IDE could dump trees to memory-mapped files and read them in on demand when needed. This was terrible for performance, but it allowed us to still run in 32bits.

The IDE has not used this in many years (since VS moved to 64bit). Looking online (github search), i can't find any usages of this api either.

Note: this API cannot be used to persist data to a stream that is then read in in a later session. Nor can it be used for cross process communication. This is because the 'object reading/writing' system keeps references to in memory data in the bytes it writes out. Specifically, it keeps arrays of "registered System.Types" in memory, and then writes out indices into that array. That array isn't deterministically created. It will change from VS session to session, and will be not be the same in VS and in our OOP process.

As such, this API exists solely for a host to write things to memory mapped files, and only read it back within the same session. In other words, it only exists for that exact use case we had in roslyn when we were 32 bit.

Given that we're not 32bit, are not using this anymore, and there are no usages of this in the wild, i recommend deprecating this API. Currently, we have to have complexity in a lot of places to support it, and it accounts for ~20k lines of code we ship: #70277

Removing this will be a nice savings in metadata and IL, and it will allow us to simplify a bunch of our serialization system. Specifically, our serializtion system will not have to have an object registration system anymore. It can just be about types writing out their contents in an explicit fashion (which needs no type registration).

@CyrusNajmabadi CyrusNajmabadi added Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature Request labels Oct 6, 2023
@dotnet-issue-labeler 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 CyrusNajmabadi added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Oct 6, 2023
@CyrusNajmabadi CyrusNajmabadi changed the title Consider deprecating syntax serialization logic. Deprecate syntax serialization logic. Oct 6, 2023
@333fred
Copy link
Member

333fred commented Oct 12, 2023

API Review

  • We're ok with this deprecation
  • We'll try to get an Obsolete(warning: true) and a NFW on usage into 17.8 so we can be more sure on whether this is in use or not.

Conclusion: Approved

@333fred 333fred added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Oct 12, 2023
@CyrusNajmabadi
Copy link
Member Author

Deprecation has gone in: #70365
A later PR can remove the functionality entirely. That PR is currently in draft here: #70277

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved in API review, it can be implemented Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature Request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants