Remove pre-parse for array-based JSON #15684

strawgate · 2021-06-29T03:22:47Z

PR Summary

Removes a previously-needed check from the convertfrom-json cmdlet. It appears a pre-parsing step was added for json whose root element was an array -- because of some issues with how Newtonsoft handled issues with Arrays in deserializeObject JamesNK/Newtonsoft.Json#1930

Upon further testing this bug is no longer present in Newtonsoft.Json and this preparsing can be removed. This was previously attempted and then rolled back because newtonsoft.json failed when deserializing a [. I have no idea why we needed to double parse this to check if the contents were [ but the tests are passing and the cases previously listed as causing issues are now working.

PR Context

It makes ConvertFrom-Json and Invoke-RestMethod ~50% faster. It does not reduce memory usage because the pre-parsing was immediately tossed out after it occurred.

Benchmark for parsing a 100MB JSON blob whose root element is an array.

Master:

Run Built-in Cmdlet: Hashtable

Memory Usage: 1079.3671875 MB
Runtime: 7256.4268 ms

Run Built-in Cmdlet: PSCustomObject

Memory Usage: 1338.74609375 MB
Runtime: 10430.8258 ms

ThisPR:

Run Built-in Cmdlet: Hashtable

Memory Usage: 1077.58203125 MB
Runtime: 3885.0489 ms

Run Built-in Cmdlet: PSCustomObject

Memory Usage: 1374.69921875 MB
Runtime: 6976.1836 ms

PR Checklist

iSazonov · 2021-06-29T18:33:10Z

Have we direct test for the scenario?

strawgate · 2021-06-29T18:41:53Z

I manually tested this but it looks like a test like what was added earlier and then removed would work?

    It 'Throws an ArgumentException with an incomplete array with AsHashtable switch set to <AsHashtable>' -TestCase $testCasesWithAndWithoutAsHashtableSwitch {
        Param($AsHashtable)
        { ConvertFrom-Json '[' -AsHashtable:$AsHashtable } |
            Should -Throw -ErrorId "System.ArgumentException,Microsoft.PowerShell.Commands.ConvertFromJsonCommand"
        { ConvertFrom-Json '["1",' -AsHashtable:$AsHashtable } |
            Should -Throw -ErrorId "System.ArgumentException,Microsoft.PowerShell.Commands.ConvertFromJsonCommand"
    }

Thoughts?

strawgate · 2021-06-29T21:04:15Z

Have we direct test for the scenario?

I added a test for this scenario

ghost · 2021-07-07T14:00:49Z

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

strawgate · 2021-07-13T20:38:23Z

I believe that this is ready to merge.

strawgate · 2021-08-10T20:33:56Z

I think this is ready if any of the pending reviewers have any questions

pull-request-quantifier-deprecated · 2021-11-30T19:57:48Z

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

Quantification details

Label      : Extra Small
Size       : +4 -2
Percentile : 2.4%

Total files changed: 2

Change summary by file extension:
.cs : +0 -2
.ps1 : +4 -0

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 detetcted.
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.

anmenaga · 2021-11-30T19:58:55Z

Reopened PR to reset hanging PR checks.

anmenaga · 2021-11-30T21:14:05Z

/azp run

azure-pipelines · 2021-11-30T21:14:42Z

Azure Pipelines successfully started running 5 pipeline(s).

ghost · 2021-12-08T02: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

daxian-dbw · 2022-07-25T19:24:28Z

/rebase

github-actions · 2022-07-25T19:25:09Z

Started rebase: https://github.com/PowerShell/PowerShell/actions/runs/2734744349

GitHub
Remove pre-parse for array-based JSON · PowerShell/PowerShell@3d4e294
PowerShell for every system! Contribute to PowerShell/PowerShell development by creating an account on GitHub.

daxian-dbw · 2022-07-25T20:43:16Z

The pre-parsing was previously removed by #8346, but then a regression was found because a feature-level test failed: #8346 (comment), and hence the change was reverted (#8375).

I have verified that the previously failing feature-level test passes with changes in this PR. So, it looks to me we are safe to merge this PR.

PowerShell/test/powershell/Modules/Microsoft.PowerShell.Utility/Json.Tests.ps1

Lines 1698 to 1711 in 3d4e294

    
           It "ConvertFrom-Json deserializes an array of PSObjects (in multiple lines) as a single string." { 
        
               # Create an array of PSCustomObjects, and serialize it 
        
               $array = [pscustomobject]@{ objectName = "object1Name"; objectValue = "object1Value" }, 
        
                        [pscustomobject]@{ objectName = "object2Name"; objectValue = "object2Value" } 
        
               # Serialize the array to a text file 
        
               $filePath = Join-Path $TESTDRIVE test.json 
        
               $array | ConvertTo-Json | Out-File $filePath -Encoding utf8 
        
               # Read the object as an array of PSObjects and deserialize it. 
        
               $result = Get-Content $filePath | ConvertFrom-Json 
        
               $result.Count | Should -Be 2 
        
           }

strawgate · 2022-07-26T00:01:27Z

Great! Thank you!

daxian-dbw · 2022-07-26T00:54:54Z

Thank you, @strawgate, for the contribution!

ghost · 2022-08-12T00:00:44Z

🎉v7.3.0-preview.7 has been released which incorporates this pull request.:tada:

Handy links:

Release Notes

Remove preparse for array-based json

cad3493

ghost assigned anmenaga Jun 29, 2021

strawgate mentioned this pull request Jun 29, 2021

ConvertFrom-JSON high memory consumption #7698

Closed

iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Jun 29, 2021

strawgate mentioned this pull request Jun 29, 2021

Introduce streaming for ConvertFrom-Json #15695

Closed

22 tasks

Adding test coverage

3e60e8e

iSazonov approved these changes Jun 30, 2021

View reviewed changes

iSazonov requested review from SteveL-MSFT and TravisEz13 June 30, 2021 03:24

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

anmenaga closed this Nov 30, 2021

ghost removed the Review - Needed The PR is being reviewed label Nov 30, 2021

anmenaga reopened this Nov 30, 2021

pull-request-quantifier-deprecated bot added the Extra Small label Nov 30, 2021

ghost added the Review - Needed The PR is being reviewed label Dec 8, 2021

ghost removed the Review - Needed The PR is being reviewed label Jul 25, 2022

daxian-dbw closed this Jul 25, 2022

daxian-dbw reopened this Jul 25, 2022

daxian-dbw merged commit ec0dba4 into PowerShell:master Jul 25, 2022

daxian-dbw assigned daxian-dbw and unassigned anmenaga Jul 25, 2022

TravisEz13 mentioned this pull request Sep 30, 2022

Move ApiScan to compliance build #18191

Merged

22 tasks

Remove pre-parse for array-based JSON #15684

Remove pre-parse for array-based JSON #15684

Uh oh!

Conversation

strawgate commented Jun 29, 2021 • edited by anmenaga 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 29, 2021

Uh oh!

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

Uh oh!

Uh oh!

strawgate commented Jun 29, 2021

Uh oh!

ghost commented Jul 7, 2021

Uh oh!

strawgate commented Jul 13, 2021

Uh oh!

strawgate commented Aug 10, 2021

Uh oh!

pull-request-quantifier-deprecated bot commented Nov 30, 2021

What can I do to optimize my changes

How to interpret the change counts in git diff output

Uh oh!

anmenaga commented Nov 30, 2021

Uh oh!

anmenaga commented Nov 30, 2021

Uh oh!

azure-pipelines bot commented Nov 30, 2021

Uh oh!

ghost commented Dec 8, 2021

Uh oh!

daxian-dbw commented Jul 25, 2022

Uh oh!

github-actions bot commented Jul 25, 2022 • edited by unfurl-links bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daxian-dbw commented Jul 25, 2022

Uh oh!

strawgate commented Jul 26, 2022

Uh oh!

daxian-dbw commented Jul 26, 2022

Uh oh!

ghost commented Aug 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

strawgate commented Jun 29, 2021 •

edited by anmenaga

Loading

strawgate commented Jun 29, 2021 •

edited

Loading

github-actions bot commented Jul 25, 2022 •

edited by unfurl-links bot

Loading