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

Reduce string allocations during SimpleJson.ParseString #2977

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ToddGrun
Copy link

@ToddGrun ToddGrun commented Oct 26, 2024

Noticed this allocation while looking at a profile of solution load in visual studio. These StringBuilder allocations were showing up as 0.5% of total allocations. I took a peek at the code, and reaslized the StringBuilder need not be created unless there is a '' in the string value being parsed. In that case, just copy already seen characters into a StringBuilder and continue as previously.

Resolves 2976


Before the change?

  • String parsing was always allocating a StringBuilder.

After the change?

  • Changed to only allocate if there are escaped characters in the json.

Pull request checklist

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

  • Yes
  • No

Noticed this allocation while looking at a profile of solution load in visual studio. These StringBuilder allocations were showing up as 0.5% of total allocations. I took a peek at the code, and reaslized the StringBuilder need not be created unless there is a '\' in the string value being parsed. In that case, just copy already seen characters into a StringBuilder and continue as previously.
@ToddGrun
Copy link
Author

@kfcampbell -- are you a good person to work with for this?

@ToddGrun
Copy link
Author

ToddGrun commented Nov 5, 2024

@kfcampbell -- Is there someone else that is more appropriate to work with on this?

@ToddGrun
Copy link
Author

ToddGrun commented Nov 9, 2024

@shiftkey -- can you help me find someone to work with to help this proceed. It's a fairly small allocation improvement that is showing up in VisualStudio profiles.

@ToddGrun
Copy link
Author

@shiftkey -- can you provide some guidance for me on how to proceed?

@ToddGrun
Copy link
Author

@MatisseHack -- it looks like you recently were able to merge a PR, so it seems like this codebase is still active. Any suggestions on how I can proceed with this?

@ToddGrun
Copy link
Author

ToddGrun commented Dec 20, 2024

@kfcampbell @shiftkey @MatisseHack -@nickfloyd - Can somebody please respond? This is getting frustrating. I've found a performance issue, tracked it down and created a fix, and have been pushing for almost 2 months for someone to respond. Should this codebase not be taken as a dependency?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

[BUG]: Unnecessary allocations during json string parsing
2 participants