-
Notifications
You must be signed in to change notification settings - Fork 676
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
Improve MSBuild SDK resolver by parsing global.json with System.Runtime.Serialization.Json #3157
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the PR title is wrong. It's using System.Runtime.Serialization.Json, not System.Xml. The Json APIs might use types from System.Xml, but we're not using XML APIs to read the JSON file.
src/NuGet.Core/Microsoft.Build.NuGetSdkResolver/GlobalJsonReader.cs
Outdated
Show resolved
Hide resolved
{ | ||
return string.IsNullOrEmpty(path) || Path.DirectorySeparatorChar == '\\' ? path : path.Replace('\\', '/'); | ||
using (var stream = new FileStream(path, FileMode.Open, FileAccess.Read, FileShare.Read)) | ||
using (var reader = System.Runtime.Serialization.Json.JsonReaderWriterFactory.CreateJsonReader(stream, XmlDictionaryReaderQuotas.Max)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given NuGet is already targeting .NET Core 3.1 and later, is there a benefit to using System.Text.Json's JsonReader instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For .NET Framework, we'd need to ship a separate assembly which I don't think would JIT as fast. The size of global.json
is very small and so the performance gain mainly comes from not having to load a big DLL to read it. So I think System.Text.Json
would probably not be a good candidate for this. Ideally we'd use some native library to read it so its even faster.
Its definitely confusing. I'm using |
98cac90
to
40a33b8
Compare
This uses the System.Runtime.Serialization.Json.JsonReaderWriterFactory to load global.json
Delete redundant TestEnvironment class in favor of our own TestDirectory class
40a33b8
to
505baf9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -1,1301 +0,0 @@ | |||
// Copyright (c) .NET Foundation. All rights reserved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔪
…em.Runtime.Serialization.Json (NuGet#3157)" This reverts commit 5a3eeab.
This is a proposed replacement of #2879. This PR uses a built-in class to read
global.json
so we still get validation of the file's schema but we don't need to loadNewtonsoft.Json
and other assemblies.The perf gain isn't as great as using a Regex but it does reduce the resolve time from ~288ms to 100ms.
Bug
Addresses: dotnet/msbuild#4025
Regression: No
Fix
Details: Use the built-in
System.Runtime.Serialization.Json.JsonReaderWriterFactory
to readglobal.json
instead ofNewtonsoft.Json
. The built-in libraries are always NGen'd/CrossGen'd and will load faster.I also cleaned up the unit tests to use NuGet's built in test utility instead of the one we added when the SDK resolver was copied here from the MSBuild repo
Testing/Validation
Tests Added: Yes
Reason for not adding tests:
Validation: