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

Proposal: ability to marshal with flags #120

Open
Jerska opened this issue Feb 17, 2022 · 3 comments
Open

Proposal: ability to marshal with flags #120

Jerska opened this issue Feb 17, 2022 · 3 comments
Assignees
Labels
enhancement New feature or request

Comments

@Jerska
Copy link
Contributor

Jerska commented Feb 17, 2022

Hi! Me again with another proposal!

json.Parse offers a readily available alternative to json.Unmarshal with the simple addition of flags.

In comparison, json.Append takes two extra parameters compared to json.Marshal: a []byte to fill and the flags.
This means that unfortunately, you can't benefit from the encoderBufferPool if you just want to add some AppendFlags.

This would give 3 functions from most to least flexible :

  • Append(b []byte, x interface {}, flags AppendFlags) - can use tags, no extra copy, but no buffer pool
  • MarshalWithFlags(x interface{}, flags AppendFlags) - can use tags, buffer pool & one extra copy
  • Marshal(x interface{}) - cannot use tags, buffer pool & one extra copy

Having to pre-allocate a buffer for Append might be quite a challenge, because you often don't know how long the resulting buffer will be.
For instance, allocating a full page like the buffer pool does is not necessarily a great option for small payloads.
Reimplementing a similar pool in our code is definitely an option, but i'd be great to simply be able to reuse the logic that's already there in the lib.

Would this be something you'd consider adding in? Again, happy to propose a PR.

@Jerska
Copy link
Contributor Author

Jerska commented Feb 24, 2022

Just to make sure someone sees this, cc @achille-roussel .

I'd understand that you don't want to modify the API too much, feel free to close if you don't believe this would be of interest.

@achille-roussel
Copy link
Contributor

Thanks for bumping this in my inbox!

If I'm understanding correctly, the core issue is that we cannot leverage the encoder buffer pool in json.Append?

I'm curious if you have used a workaround in your application today?

@achille-roussel achille-roussel self-assigned this Feb 24, 2022
@achille-roussel achille-roussel added the enhancement New feature or request label Feb 24, 2022
@Jerska
Copy link
Contributor Author

Jerska commented Feb 24, 2022

Thank you for your response!

If I'm understanding correctly, the core issue is that we cannot leverage the encoder buffer pool in json.Append?

Exactly !

I'm curious if you have used a workaround in your application today?

I've managed to mostly use json.Append for things where I could approximate the buffer size I needed to pre-allocate.

However, for a few others, I've resorted to using json.Marshal.
For those structures the benefits of the preallocated buffer are greater than the optimizations provided by removing SortKeys and EscapeHTML.
It would be more optimized for me to manually handle a buffer pool on our side and use json.Append, but this kind of additions wouldn't fit our criteria for complexity / perf improvements.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants