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

refactor: use Newtonsoft.Json streaming methods #596

Merged
merged 1 commit into from
Jun 6, 2023

Conversation

JamieMagee
Copy link
Member

As we're unable to move to System.Text.Json with the feature set that is available in .NET 6 (See #231 for more information), this PR optimizes our use of Newtonsoft.Json by using streaming methods for our most allocation-heavy methods where possible.

In tests against real world repositories this dropped memory usage by up to 37% when writing the manifest JSON.

There is also some refactoring and tech debt cleanup in tests and documentation.

@JamieMagee JamieMagee requested a review from a team as a code owner June 6, 2023 19:10
@JamieMagee JamieMagee requested a review from melotic June 6, 2023 19:10
@melotic
Copy link
Member

melotic commented Jun 6, 2023

LGTM! Although I don't think this affects the manfiest, might be good to just double check that the format is still the same.

@JamieMagee JamieMagee merged commit cb6204f into main Jun 6, 2023
@JamieMagee JamieMagee deleted the users/jamagee/stream-json branch June 6, 2023 20:28
@JamieMagee JamieMagee changed the title feat: use Newtonsoft.Json streaming methods refactor: use Newtonsoft.Json streaming methods Jun 6, 2023
@github-actions
Copy link

github-actions bot commented Jun 6, 2023

👋 Hi! It looks like you modified some files in the Detectors folder.
You may need to bump the detector versions if any of the following scenarios apply:

  • The detector detects more or fewer components than before
  • The detector generates different parent/child graph relationships than before
  • The detector generates different devDependencies values than before

If none of the above scenarios apply, feel free to ignore this comment 🙂

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.

2 participants