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

Serialize Razor code document #8832

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jjonescz
Copy link
Contributor

@jjonescz jjonescz commented Jun 15, 2023

Resolves #8643.

cc @chsienki

@DustinCampbell
Copy link
Member

FWIW, it worries me a bit that we're going further down the rabbit hole of using JSON.NET and lots of JsonConverters. We're actively trying to get away from that in the tooling layers for performance and lower allocation. Is there any reason to use JSON.NET here? Or, could this use System.Text.Json or MessagePack? Or, is the intent to just get things working now and rework it later?

@jjonescz
Copy link
Contributor Author

jjonescz commented Jun 15, 2023

We're actively trying to get away from that in the tooling layers for performance and lower allocation.

I wrote this code some time ago and did not know this back then.

is the intent to just get things working now and rework it later?

So I guess this is the current intent.

Is there any reason to use JSON.NET here

Wanted to use the existing TagHelperDescriptor converters. Although now there are more converters in the tooling layer, when I started I'm not sure if they weren't there or I didn't know about them, anyway this PR basically uses some other (legacy) converters from compiler layer. Specifically these ones:

<ItemGroup>
<Compile Include="..\shared\JsonReaderExtensions.cs" LinkBase="Shared" />
<Compile Include="..\shared\RazorDiagnosticJsonConverter.cs" LinkBase="Shared" />
<Compile Include="..\shared\TagHelperDescriptorJsonConverter.cs" LinkBase="Shared" />
</ItemGroup>

@DustinCampbell
Copy link
Member

Wanted to use the existing TagHelperDescriptor converters

FWIW, there is no longer a TagHelperDescriptor converter in the tooling layer. Instead, there are only JsonConverters for "top-level" types -- i.e.., objects that are passed as arguments or results via StreamJsonRpc or serialized directly (like ProjectRazorJson). None of the tooling JsonConverters call into JsonSerializer.Serialize(...) or JsonSerializer.Deserialize(...).

The goal for serialization in the tooling layer is to have an abstraction that we can use (in the near future) to dedupe common object instances and strings in the data, and (again later) switch to a more efficient serialization system. I hope we can just build on this work eventually, though I totally get wanting to just get things working now. This an area that I've actively been working on due to performance analysis that have pointed to our serialization.

@jjonescz jjonescz force-pushed the HostOutputs-SerializeDoc branch 4 times, most recently from e5c9ca0 to fcdbecf Compare June 16, 2023 10:39
@@ -5,7 +5,9 @@
<IgnorePatterns>
<UsagePattern IdentityGlob="Microsoft.SourceBuild.Intermediate.*/*" />

<!--
<Usage Id="Newtonsoft.Json" Version="13.0.3" />
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dotnet/source-build-internal how do I fix "new prebuilts detected" for Newtonsoft.Json? Is this fine?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for asking @jjonescz. Source build must build all dependent binaries from source. External dependencies such as Newtonsoft.Json are contained in the https://github.com/dotnet/source-build-externals repo. To fix this prebuilt, you simply need to add the appropriate version.details.xml dependency to this repo and add a darc subscription to keep it up to date.

    <Dependency Name="Microsoft.SourceBuild.Intermediate.source-build-externals" Version="8.0.0-alpha.1.23312.1">
      <Uri>https://github.com/dotnet/source-build-externals</Uri>
      <Sha>50600f7fcd6e56a496233a859ed7d4d90c173b0d</Sha>
      <SourceBuild RepoName="source-build-externals" ManagedOnly="true" />
    </Dependency>

When razor CI runs the source-build leg, this package will be pulled in by the source-build infrastructure from source-build-externals and therefore will not be considered a prebuilt.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what I originally tried, but it doesn't get rid of the error. I redid the change, so you can see - it fails currently in CI. Can the reason be that I'm using Newtonsoft.Json version 13.0.3 but there's 13.0.1 in source-build-externals (I think)? But using 13.0.1 explicitly would get NuGet complaining about package downgrade (since there's Newtonsoft.Json 13.0.3 flowing into other projects from Roslyn) - although I guess I could use 13.0.1 for this source-built project and keep 13.0.3 for others if that will solve the problem...

@jjonescz jjonescz marked this pull request as ready for review June 16, 2023 11:19
@jjonescz jjonescz requested review from a team as code owners June 16, 2023 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make RazorCodeDocument serializable
3 participants