Use new JSON Streaming methods for Invoke-RestMethod #15697

strawgate · 2021-06-30T01:26:09Z

PR Summary

This PR is a "child" PR to: #15695

This PR is in good shape but needs tests for the additional parameter on invoke-restmethod for -ashashtable

Essentially if the streaming approach from that PR is approved we can make new methods which take a stream directly instead of the string body. We can then wire up invoke-restmethod so that it takes the response stream and sends it directly to newtonsoft.

This should improve all use-cases of invoke-restmethod but will particularly improve cases where the source API has an array as its root element.

PR Context

This should greatly reduce the memory usage of the invoke-restmethod cmdlets and somewhat reduce the execution time/cpu time.

Benchmarks for a 100MB JSON Response with Array root element (Time to first byte from the API is about 8s so for realistic times subtract 8000ms from each runtime number)

7.1.3:

Memory Usage: 1597 MB
Runtime: 20679.9797 ms
Est Runtime: 12679.9797 ms

ThisPR:

Memory Usage: 718.55 MB
Runtime: 16323.1985 ms
Est Runtime: 8323.1985 ms

ThisPR -AsHashtable:

Memory Usage: 415.41 MB
Runtime: 13592.6211 ms
Est Runtime: 5592.6211 ms

Benchmarks for a 100MB JSON response with Object root element
7.1.3

Memory Usage: 1672.84765625 MB
Runtime: 12771.3602 ms
Est Runtime: 12771.3602 ms

This PR:

Memory Usage: 1489.90625 MB
Runtime: 12222.5628 ms

This PR -AsHashtable

Memory Usage: 1146.078125 MB
Runtime: 11367.9809 ms

Benchmarks for a 12MB JSON Response
7.1.3:

Memory Usage: 268.9921875 MB
Runtime: 2553.9797 ms

ThisPR:

Memory Usage: 100.05078125 MB
Runtime: 1950.1985 ms

Runtime is misleading as I am hitting a real API

PR Checklist

…lets pass it directly instead of having to worry about properties

…zer settings

iSazonov · 2021-06-30T03:41:41Z

@strawgate I believe we have had a consensus that we froze current Json cmdlets based on NewtonSoft and implement new ones based on .Net API as experimental feature.

strawgate · 2021-06-30T03:58:12Z

I looked around a fair amount for information on the status of System.Text.Json and all I found was a couple 2 year old PRs that haven't moved.

My goal was to offer a highly-compatible set of changes that make a big impact on performance while we wait for System.Text.Json. The changes outlined here cut memory usage to 1/4 of what it was previously if the root element is an array and ~75% of what it was if it's an object.

The high memory usage of Powershell with API use-cases was the primary reason we moved most of our projects to C#. I've talked with a lot of people who have moved to C# or Python due to the overhead related to parsing JSON. Instead of complaining I figured i'd take a stab at it and get some experience working with the codebase.

If you guys don't want to merge the changes that's totally fine!

iSazonov · 2021-06-30T04:55:25Z

@strawgate The main problem is that the MSFT team is too small to review everything PR quickly.
They introduced Working Groups to get more developers from the community. If you want to work on commandlets in this repository, you can join one of the working groups.

The current strategic direction in the C# community is migration from NewtonSoft.JsonNet to System.Text.Json API. NewtonSoft has been found to be out of development. All new developers should be on System.Text.Json API.

It doesn't make sense to invest much in code based on NewtonSoft.JsonNet, only security issues, simple bugs/perfs.

If you are interested in working on these commandlets, the current conclusion is to implement new commandlets based on the System.Text.Json API as experimental features. You can grab my PRs then I could review your PRs.

…s prevents errors when a null is passed through.

strawgate · 2021-06-30T15:43:53Z

I've been using https://www.mockaroo.com/ to generate random json data with numbers, dates, etc and have been using http://www.jsondiff.com/ to do a semantic compare between the 7.1.3 cmdlets and the ones in this PR by converting it from json and then back to json and comparing the two. So far so good!

strawgate · 2021-07-01T22:59:20Z

@strawgate The main problem is that the MSFT team is too small to review everything PR quickly.
They introduced Working Groups to get more developers from the community. If you want to work on commandlets in this repository, you can join one of the working groups.

The current strategic direction in the C# community is migration from NewtonSoft.JsonNet to System.Text.Json API. NewtonSoft has been found to be out of development. All new developers should be on System.Text.Json API.

It doesn't make sense to invest much in code based on NewtonSoft.JsonNet, only security issues, simple bugs/perfs.

If you are interested in working on these commandlets, the current conclusion is to implement new commandlets based on the System.Text.Json API as experimental features. You can grab my PRs then I could review your PRs.

I can't find information on joining a specific working group, is that info available somewhere?

iSazonov · 2021-07-02T05:06:54Z

I can't find information on joining a specific working group, is that info available somewhere?

You could e-mail directly @joeyaiello and @SteveL-MSFT.

ghost · 2021-07-09T14:00:43Z

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

TravisEz13

preliminary review

TravisEz13 · 2021-07-12T19:11:29Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs

            }
        }

+        private static List<object> DeserializeRootArrayHelper(JsonReader reader, JsonSerializer serializer, bool returnHashtable, out ErrorRecord error)


Add description of the method and parameters

TravisEz13 · 2021-07-12T19:17:30Z

Converted to draft, please marks as ready for review when the parent PR is merged.

strawgate · 2021-07-12T20:32:39Z

I think this PR will be abandoned as there is no interest in improving the performance of the existing cmdlets due to backwards compatibility concerns.

iSazonov · 2021-07-13T04:35:06Z

I think this PR will be abandoned as there is no interest in improving the performance of the existing cmdlets due to backwards compatibility concerns.

All this could be in new experimental cmdlets based on System.Text.Json API.

strawgate added 11 commits June 28, 2021 22:15

Remove preparse for array-based json

cad3493

WIP POC

2948d04

improve token handling and if the first element is not an array then …

db2a47b

…lets pass it directly instead of having to worry about properties

Fold back into using a serializer so we can use the requested seriali…

c2ecea3

…zer settings

Fixing some failing tests and responding to a PR comment

9416881

Remove commented out function

7039296

Code factor cleanup

f197057

Add some comments, additional empty line removal

f873392

Feedback from PR comments

e2de68f

Cleanup line for code factor

1ceaf24

Wire up invoke-restmethod with new json conversion streams

5056eb2

ghost assigned TravisEz13 Jun 30, 2021

refactor from testing, add support for asHashtable to invoke-restmethod

c52ceb4

strawgate changed the title ~~WIP: Use new JSON Streaming methods for Invoke-RestMethod~~ Use new JSON Streaming methods for Invoke-RestMethod Jun 30, 2021

strawgate added 2 commits June 29, 2021 23:43

Fixes from CI testing

15dbebc

Did not mean to commit these changes

8b1756d

Dont use the same names for stream and string json deserializers. Thi…

bddf4d8

…s prevents errors when a null is passed through.

ghost added the Review - Needed The PR is being reviewed label Jul 9, 2021

TravisEz13 requested changes Jul 12, 2021

View reviewed changes

ghost added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Review - Needed The PR is being reviewed labels Jul 12, 2021

TravisEz13 marked this pull request as draft July 12, 2021 19:16

strawgate closed this Jul 12, 2021

ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Jul 12, 2021

Use new JSON Streaming methods for Invoke-RestMethod #15697

Use new JSON Streaming methods for Invoke-RestMethod #15697

Uh oh!

Conversation

strawgate commented Jun 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Context

PR Checklist

Uh oh!

iSazonov commented Jun 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

strawgate commented Jun 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented Jun 30, 2021

Uh oh!

strawgate commented Jun 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

strawgate commented Jul 1, 2021

Uh oh!

iSazonov commented Jul 2, 2021

Uh oh!

ghost commented Jul 9, 2021

Uh oh!

TravisEz13 left a comment

Choose a reason for hiding this comment

Uh oh!

TravisEz13 Jul 12, 2021

Choose a reason for hiding this comment

Uh oh!

TravisEz13 commented Jul 12, 2021

Uh oh!

strawgate commented Jul 12, 2021

Uh oh!

iSazonov commented Jul 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

strawgate commented Jun 30, 2021 •

edited

Loading

iSazonov commented Jun 30, 2021 •

edited

Loading

strawgate commented Jun 30, 2021 •

edited

Loading

strawgate commented Jun 30, 2021 •

edited

Loading