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

Use System.Text.Json instead of Newton.Json #525

Merged
merged 12 commits into from
Feb 8, 2023

Conversation

dbalikhin
Copy link
Contributor

Fixes #515

@dbalikhin
Copy link
Contributor Author

@microsoft-github-policy-service agree

@gfs
Copy link
Contributor

gfs commented Feb 8, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@gfs
Copy link
Contributor

gfs commented Feb 8, 2023

Thanks for your contribution. This is an excellent improvement I think.

I added a simple benchmark to test the speed of just the export time. It generates a trivial report with N issues for setup and then exports the same report to JSON repeatedly. The benchmark shows about a 2x speedup, and about 10x less memory allocations.

Current main branch

Method N Mean Error StdDev Ratio Gen0 Allocated Alloc Ratio
ExportRecordsToJson 1000 3.680 ms 0.0736 ms 0.1833 ms 1.00 23.4375 387.88 KB 1.00
ExportRecordsToJson 10000 30.532 ms 0.6079 ms 1.5138 ms 1.00 218.7500 3762.89 KB 1.00

This PR

Method N Mean Error StdDev Ratio Gen0 Allocated Alloc Ratio
ExportRecordsToJson 1000 2.052 ms 0.0494 ms 0.1442 ms 1.00 - 52.17 KB 1.00
ExportRecordsToJson 10000 13.035 ms 0.2579 ms 0.4310 ms 1.00 15.6250 480.6 KB 1.00

@gfs
Copy link
Contributor

gfs commented Feb 8, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

This PR contains public API changes so semver must be bumped.
@gfs
Copy link
Contributor

gfs commented Feb 8, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@gfs
Copy link
Contributor

gfs commented Feb 8, 2023

This is relatively large in terms of files touched so I'm going to give it another look tomorrow to double check nothing got missed but I expect to merge it sometime tomorrow.

@dbalikhin
Copy link
Contributor Author

Thanks folks, I really appreciate your responsiveness. I will check this PR if I need to modify anything.

@gfs gfs merged commit 5059422 into microsoft:main Feb 8, 2023
@gfs
Copy link
Contributor

gfs commented Feb 8, 2023

@dbalikhin I did a quick check of the json, sarif and html outputs compared to the latest release from main and everything looks good, as well as double checking that custom rules can still be specified and loaded properly.

Thanks again for the contribution, great work!

@dbalikhin
Copy link
Contributor Author

@gfs

Thanks again for the contribution, great work!

My pleasure.

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.

Refactor to use System.Text.Json
3 participants