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

Json appender #458

Merged
merged 7 commits into from
Jun 29, 2022
Merged

Json appender #458

merged 7 commits into from
Jun 29, 2022

Conversation

davidlar
Copy link
Contributor

This is a complete rework of #424 after review by @jdegoes.

@davidlar davidlar mentioned this pull request May 28, 2022
jdegoes
jdegoes previously approved these changes Jun 26, 2022
Copy link
Member

@jdegoes jdegoes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks phenomenal! Thanks for putting up with the revisions.

After conflicts are fixed, this is good to merge!

There is some separate work we could do later to optimize performance.

@jdegoes
Copy link
Member

jdegoes commented Jun 26, 2022

@davidlar This is good to merge when you get a chance to fix conflicts!

@davidlar
Copy link
Contributor Author

Great!

I thought about reusing the string builder more instead of creating new ones. Don't know if it will affect performance or not.
What were the things you were thinking about?

@jdegoes jdegoes merged commit 7c4ee88 into zio:master Jun 29, 2022
@jdegoes
Copy link
Member

jdegoes commented Jun 29, 2022

@davidlar Thanks for your persistence! We finally got this merged in.

One of the main things, I think, would be using low-level 'while' loops and working directly with Char, because the current implementation will allocate and box a lot for escaping. In addition, we could go further with using CharSequence strategically.

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