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 buffer pools for JsonSerializationWriter #81

Merged
merged 1 commit into from
Apr 26, 2023

Conversation

eric-millin
Copy link
Contributor

This PR depends on #79

Creating and garbage collecting buffers is expensive and can have a significant impact in scenarios with high concurrency. This change improves serialization performance by utilizing a buffer pool.

I also made the Close semantics more idiomatic by making it a true finalizer. The existing behavior is what most Go packages would wrap in a Reset function. The current behavior is counterintuitive because I typically wouldn’t expect to reuse a struct after I had called Close on it.

The existing Close implementation also has the disadvantage of allocating a new buffer even when there is no intent to reuse the writer. If a user is always calling Close before disposing the writer, they’re unwittingly hurting performance by doubling buffer allocations and increasing the effort required to clean them up.

Same benchmark as #79

BenchmarkNoPool-10    	  477813	      2499 ns/op	    2040 B/op	      22 allocs/op
BenchmarkPool-10     	  525110	      2277 ns/op	     648 B/op	      16 allocs/op

@baywet
Copy link
Member

baywet commented Apr 21, 2023

Thanks for the additional contribution @eric-millin. Out of curiosity, why not push that to the original PR? Also expect some delays in reviews, most of the team is based in Kenya and they've had a couple of national holidays recently.

@eric-millin
Copy link
Contributor Author

Thanks for the additional contribution @eric-millin. Out of curiosity, why not push that to the original PR? Also expect some delays in reviews, most of the team is based in Kenya and they've had a couple of national holidays recently.

I thought about it and can do, if you prefer. In my mind the change to the semantics of Close deserved a separate PR. Also, I didn't want to create confusion by tacking on code to a PR that was already approved. Finally, they're not tightly coupled: you can use a pool with the current string slice or you can use it with buffers.

But y'all's the boss, so happy to combine them if it makes life easier.

@baywet
Copy link
Member

baywet commented Apr 21, 2023

Thanks for the context, we don't want you to have to do additional work, this is fine, we'll review this one once the other one is merged.

@eric-millin
Copy link
Contributor Author

Thanks for the context, we don't want you to have to do additional work, this is fine, we'll review this one once the other one is merged.

Lmk if you change your mind. It's just a matter of git push :)

@github-actions
Copy link
Contributor

This pull request has conflicting changes, the author must resolve the conflicts before this pull request can be merged.

@baywet
Copy link
Member

baywet commented Apr 25, 2023

@eric-millin, would you mind rebasing and handling the conflicts please?

Copy link
Contributor

@rkodev rkodev left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution :-). Please resolve differenced and we should be good to go

@eric-millin
Copy link
Contributor Author

@eric-millin, would you mind rebasing and handling the conflicts please?

Done

Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. @rkodev for final review

@rkodev rkodev merged commit b68e8c5 into microsoft:main Apr 26, 2023
meain added a commit to meain/kiota-serialization-json-go that referenced this pull request May 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants