Replace Newtonsoft.Json with System.Text.Json in PSConfiguration #14411

iSazonov · 2020-12-14T05:40:11Z

PR Summary

Use System.Text.Json instead of Newtonsoft.Json in PSConfiguration.

Perf win is ~11% in startup scenario (pwsh -c exit) with two config files (user and system). It is ~60 ms on my system.

Breaking change because System.Text.Json have many minor (by design) breaking changes compared with Netonsoft.Json.
So I had to change "true" on true in powershell.config.json file in tests.

Before the PR:

After the PR:

PR Context

Related #14268

PR Checklist

ghost · 2020-12-24T02:00:13Z

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

src/System.Management.Automation/engine/InitialSessionState.cs

src/System.Management.Automation/engine/PSConfiguration.cs

iSazonov · 2021-09-18T15:25:26Z

Does Team want this in 7.2?
We could benefit from JsonNode (new in .Net 6.0).

ghost · 2021-09-26T02:00:45Z

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

src/System.Management.Automation/engine/InitialSessionState.cs

src/System.Management.Automation/engine/PSConfiguration.cs

test/powershell/Modules/Microsoft.PowerShell.Core/CompatiblePSEditions.Module.Tests.ps1

iSazonov · 2022-09-26T17:56:11Z

Rebased to move to .Net 7 RC1.

This reverts commit 7bb07da.

pull-request-quantifier-deprecated · 2022-09-26T18:14:49Z

This PR has 187 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

Quantification details

Label      : Medium
Size       : +94 -93
Percentile : 57.4%

Total files changed: 9

Change summary by file extension:
.cs : +82 -82
.ps1 : +12 -11

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

Fast and predictable releases to production:
- Optimal size changes are more likely to be reviewed faster with fewer
  iterations.
- Similarity in low PR complexity drives similar review times.
Review quality is likely higher as complexity is lower:
- Bugs are more likely to be detected.
- Code inconsistencies are more likely to be detected.
Knowledge sharing is improved within the participants:
- Small portions can be assimilated better.
Better engineering practices are exercised:
- Solving big problems by dividing them in well contained, smaller problems.
- Exercising separation of concerns within the code changes.

What can I do to optimize my changes

Use the PullRequestQuantifier to quantify your PR accurately
- Create a context profile for your repo using the context generator
- Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
- Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
- Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
Change your engineering behaviors
- For PRs that fall outside of the desired spectrum, review the details and check if:
  - Your PR could be split in smaller, self-contained PRs instead
  - Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

One line was added: +1 -0
One line was deleted: +0 -1
One line was modified: +1 -1 (git diff doesn't know about modified, it will
interpret that line like one addition plus one deletion)
Change percentiles: Change characteristics (addition, deletion, modification)
of this PR in relation to all other PRs within the repository.

Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

SteveL-MSFT

LGTM

daxian-dbw

Not finished reviewing but want to share my comments so far.

daxian-dbw · 2022-09-26T22:12:56Z

src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHost.cs

 #endif

-            if (s_cpp.SettingsFile is not null)
+            if (!string.IsNullOrEmpty(s_cpp.SettingsFile))


No need to change this. Based on how it's set in TryParseSettingFileHelper, s_cpp.SettingsFile is guaranteed to not be an empty string.

daxian-dbw · 2022-09-26T22:27:02Z

src/System.Management.Automation/engine/PSConfiguration.cs

        internal void SetSystemConfigFilePath(string value)
        {
-            if (!string.IsNullOrEmpty(value) && !File.Exists(value))
+            FileInfo info = new FileInfo(value);


Why changing the original code? You didn't guard against null value.
I know you tried to check null and empty in ConsoleHost.cs, but this is an internal method, and you cannot assume it will be invoked only from that callsite.

daxian-dbw · 2022-09-26T22:34:11Z

src/System.Management.Automation/engine/PSConfiguration.cs

-        private readonly JObject[] configRoots;
-        private readonly JObject emptyConfig;
-        private readonly JsonSerializer serializer;
+        private readonly JsonObject[] _configScopeRoots;


It seems there is no particular gains over changing the name from configRoots to _configScopeRoots. It would make reading the changes easier if you can keep the original names.
Also, regarding using the prefix _ for class members, I understand it's preferred naming pattern, but given the rest member names don't have the prefix, it would be better to not use it for this PR too to keep consistency. If wanted, a separate PR should be used to fix the code styles.

daxian-dbw · 2022-09-26T22:35:44Z

src/System.Management.Automation/engine/PSConfiguration.cs

        /// <param name="key">The string key of the value.</param>
        /// <param name="defaultValue">The default value to return if the key is not present.</param>
-        private T ReadValueFromFile<T>(ConfigScope scope, string key, T defaultValue = default)
+        [return: NotNullIfNotNull("defaultValue")]


Suggested change

[return: NotNullIfNotNull("defaultValue")]

[return: NotNullIfNotNull(nameof(defaultValue))]

daxian-dbw · 2022-09-26T22:36:32Z

src/System.Management.Automation/engine/PSConfiguration.cs

        {
            string fileName = GetConfigFilePath(scope);
-            JObject configData = configRoots[(int)scope];
+            var configScopeData = _configScopeRoots[(int)scope];


same here, keep the name configData would be helpful to review.

daxian-dbw · 2022-09-26T22:55:39Z

src/System.Management.Automation/engine/PSConfiguration.cs

                        using var stream = OpenFileStreamWithRetry(fileName, FileMode.Open, FileAccess.Read, FileShare.ReadWrite);
-                        using var jsonReader = new JsonTextReader(new StreamReader(stream));
+                        using var reader = new StreamReader(stream);
+                        string jsonString = reader.ReadToEnd();


This could potentially create LOH allocation.

daxian-dbw · 2022-09-26T23:18:44Z

I don't fully understand the gains of this work, especially given that it's a breaking change.

Breaking change because System.Text.Json have many minor (by design) breaking changes compared with Netonsoft.Json.
So I had to change "true" on true in powershell.config.json file in tests.

From the perspective of startup time improvement, how much of the gain is related to spinning up a task to warm up JSON parsing in InitialSessionState.cs? Also, it's hard to reason from the attached screenshots in the PR description. Some benchmarking similar to Improve powershell startup time #8341 (comment) would be helpful to see the perf difference.
From the perspective of mitigating assembly loading conflicts, we still have a hard dependency to Newtonsoft.Json in the Utility module, so switching to System.Text.Json in PSConfiguraiton handling doesn't really solve the problem.

iSazonov · 2022-09-27T04:34:53Z

I don't fully understand the gains of this work, especially given that it's a breaking change.

Intention is to remove dependency on Newtonsoft.Json at all. It is impossible to do this in one PR, only step by step.
Today we have active PR for Test-Json. Then we could update other two cmdlets.

Before I continue with the PR. I'd want to get a conclusion from MSFT team - do we want to rid of Newtonsoft.Json in the milestone?

daxian-dbw · 2022-09-27T05:36:48Z

do we want to rid of Newtonsoft.Json in the milestone?

I think this largely depends on whether we can retain the same behavior as of today. If breaking changes are inevitable, then I personally tend to not making the change, but of course, it would be helpful to understand the scale and risky level of those breaking changes.

Besides, I have 2 more concerns:

Completely avoid Newtonsoft.Json is helpful to some extent to the assembly conflict issue, but it won't really solve the problem -- too many modules are depending on that library, which may use different versions due to different cadency of the releases. So, IMO, educating module authors to avoid the problem for their modules is the right way to go (hence I made https://github.com/daxian-dbw/PowerShell-ALC-Samples).
Some modules assume that PowerShell 7.x ships with Newtonsoft.Json, and removing that dependency is a breaking change to those modules.

iSazonov · 2022-09-27T06:03:00Z

I think this largely depends on whether we can retain the same behavior as of today.

.Net implementation fully follows JSON standard. So users cannot say that PowerShell cmdlets do not work correctly.
We have a lot of tests to help us understand how many differences there are. See #18141 for example.

Current JSON cmdlets could be published on PowerShellGet site in a compatibility module.

Some modules assume that PowerShell 7.x ships with Newtonsoft.Json, and removing that dependency is a breaking change to those modules.

It seems .Net continue distribute Newtonsoft.Json.

ghost · 2022-10-04T14:01:23Z

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

floh96 · 2022-10-04T14:17:13Z

@daxian-dbw can you mark this issue for Committee Review to get a conclusion and move this forward?

daxian-dbw · 2022-10-04T16:24:52Z

@PowerShell/powershell-committee Please review to decide whether this PR is desired. Please see the comments starting from #14411 (comment) for reference and context.

SteveL-MSFT · 2022-10-05T21:25:54Z

@PowerShell/powershell-committee reviewed this. We agree that this is the right change towards removing dependency on NewtonSoft the for the engine to read powershell.config.json. The breaking change for powershell.config.json seems unlikely to hit real world scenarios due to differences we understand for NewtonSoft. Additional changes in JSON cmdlets, for example, need to be evaluated on a case-by-case basis so this statement isn't an automatic acceptance of further changes. We also expect that we cannot immediately remove shipping NewtonSoft even if PS7 itself removes all dependencies and need a migration story for modules (and potentially scripts) that may explicitly depend on it.

ghost · 2022-10-13T02:01:25Z

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

iSazonov · 2022-10-13T12:15:08Z

After merging SG for PSVersionTable the config reading takes 20% in startup scenario.

ghost · 2022-10-20T14:01:28Z

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

iSazonov added the CL-Performance Indicates that a PR should be marked as a performance improvement in the Change Log label Dec 14, 2020

iSazonov assigned daxian-dbw Dec 14, 2020

iSazonov mentioned this pull request Dec 14, 2020

Startup performance PowerShell 7.2.0-Preview.1 #14268

Closed

15 tasks

iSazonov changed the title ~~Replace Newtonsoft.Json with System.Text.Json~~ Replace Newtonsoft.Json with System.Text.Json in PSConfiguration Dec 14, 2020

iSazonov added Breaking-Change breaking change that may affect users CL-BreakingChange Indicates that a PR should be marked as a breaking change in the Change Log labels Dec 15, 2020

iSazonov requested a review from daxian-dbw December 16, 2020 04:34

SteveL-MSFT added the Backport-7.2.x-Consider label Dec 16, 2020

ghost added the Review - Needed The PR is being reviewed label Dec 24, 2020

daxian-dbw requested review from adityapatwardhan and rjmholt September 15, 2021 18:45

adityapatwardhan removed the Backport-7.2.x-Consider label Sep 15, 2021

ghost removed the Review - Needed The PR is being reviewed label Sep 15, 2021

rjmholt reviewed Sep 17, 2021

View reviewed changes

ghost added the Review - Needed The PR is being reviewed label Sep 26, 2021

iSazonov force-pushed the json-psconfig2 branch from 7722939 to accb74a Compare October 18, 2021 09:26

iSazonov requested review from TravisEz13 and anmenaga as code owners October 20, 2021 17:47

iSazonov force-pushed the json-psconfig2 branch 5 times, most recently from 62944a8 to ff0e610 Compare October 29, 2021 17:53

pull-request-quantifier-deprecated bot added the Medium label Oct 29, 2021

SteveL-MSFT reviewed Aug 1, 2022

View reviewed changes

src/System.Management.Automation/engine/InitialSessionState.cs Outdated Show resolved Hide resolved

SteveL-MSFT reviewed Aug 1, 2022

View reviewed changes

src/System.Management.Automation/engine/PSConfiguration.cs Outdated Show resolved Hide resolved

SteveL-MSFT reviewed Aug 1, 2022

View reviewed changes

test/powershell/Modules/Microsoft.PowerShell.Core/CompatiblePSEditions.Module.Tests.ps1 Outdated Show resolved Hide resolved

iSazonov force-pushed the json-psconfig2 branch from cce9e62 to 7bb07da Compare September 26, 2022 17:55

ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Sep 26, 2022

Revert "Remove using"

e5424ac

This reverts commit 7bb07da.

SteveL-MSFT approved these changes Sep 26, 2022

View reviewed changes

daxian-dbw requested changes Sep 26, 2022

View reviewed changes

ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Sep 26, 2022

ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Sep 27, 2022

ghost added the Review - Needed The PR is being reviewed label Oct 4, 2022

daxian-dbw added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Oct 4, 2022

ghost removed the Review - Needed The PR is being reviewed label Oct 4, 2022

SteveL-MSFT added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Oct 5, 2022

ghost added the Review - Needed The PR is being reviewed label Oct 13, 2022

ghost removed the Review - Needed The PR is being reviewed label Oct 13, 2022

ghost added the Review - Needed The PR is being reviewed label Oct 20, 2022

iSazonov closed this Apr 23, 2023

ghost removed the Review - Needed The PR is being reviewed label Apr 23, 2023

	[return: NotNullIfNotNull("defaultValue")]
	[return: NotNullIfNotNull(nameof(defaultValue))]

Replace Newtonsoft.Json with System.Text.Json in PSConfiguration #14411

Replace Newtonsoft.Json with System.Text.Json in PSConfiguration #14411

Uh oh!

Conversation

iSazonov commented Dec 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Context

PR Checklist

Uh oh!

ghost commented Dec 24, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

iSazonov commented Sep 18, 2021

Uh oh!

ghost commented Sep 26, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

iSazonov commented Sep 26, 2022

Uh oh!

pull-request-quantifier-deprecated bot commented Sep 26, 2022

What can I do to optimize my changes

How to interpret the change counts in git diff output

Uh oh!

SteveL-MSFT left a comment

Choose a reason for hiding this comment

Uh oh!

daxian-dbw left a comment

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Sep 26, 2022

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Sep 26, 2022

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Sep 26, 2022

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Sep 26, 2022

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Sep 26, 2022

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Sep 26, 2022

Choose a reason for hiding this comment

Uh oh!

daxian-dbw commented Sep 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented Sep 27, 2022

Uh oh!

daxian-dbw commented Sep 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented Sep 27, 2022

Uh oh!

ghost commented Oct 4, 2022

Uh oh!

floh96 commented Oct 4, 2022

Uh oh!

daxian-dbw commented Oct 4, 2022

Uh oh!

SteveL-MSFT commented Oct 5, 2022

Uh oh!

ghost commented Oct 13, 2022

Uh oh!

iSazonov commented Oct 13, 2022

Uh oh!

ghost commented Oct 20, 2022

Uh oh!

Reviewers

Assignees

iSazonov commented Dec 14, 2020 •

edited

Loading

daxian-dbw commented Sep 26, 2022 •

edited

Loading

daxian-dbw commented Sep 27, 2022 •

edited

Loading