Skip to content

Support get_bytes#4

Open
deankarn wants to merge 6 commits into
tidwall:mainfrom
deankarn:support-bytes
Open

Support get_bytes#4
deankarn wants to merge 6 commits into
tidwall:mainfrom
deankarn:support-bytes

Conversation

@deankarn

@deankarn deankarn commented Mar 20, 2022

Copy link
Copy Markdown

The main goal of this PR was to add a new function gjson::get_bytes(...) to compliment the gjson::get(...) preventing the need to convert to a string, but noticed a few other things while doing that so the highlights are:

  • Added gjson::get_bytes(...) to compliment the gjson::get(...) here
  • Simplified and reduced size of Value by using Cow combining the slice and owned fields into one and deferring escaping strings until needed/used, see here
  • Converted a bunch of functions that accepted a &str and then immediately called as_bytes() on that string to accept a &[u8] directly which has reduced the need to convert between these types as well as returning Vec<u8> instead of String to help facilitate. See here for example.
  • Cleaned up some checks to be more idiomatic rust eg. !data.is_empty() vs data.len() > 0 for readability.

These changes, for a project I'm using gjson in, benchmarked 5-10% faster in some situations when fetching values.

@deankarn

deankarn commented Mar 29, 2022

Copy link
Copy Markdown
Author

@tidwall is there anything else needed for this PR?

P.S. Also if you feel it's too much I can make a simpler one only for supporting get_bytes in the meantime if that helps as I need it to deploy my crate :)

@tidwall

tidwall commented Mar 30, 2022

Copy link
Copy Markdown
Owner

Hi Dean, I appreciate the time you spent on the PR. I've only taken a cursory look and from what I can see there's some performance optimizations, which is totally awesome, but unfortunately I currently don't the time needed to do a thorough review. Hopefully in the near future. I'm not sure about the logistics of your current work, but in the meantime could there be a way to maintain a forked crate?

@deankarn

Copy link
Copy Markdown
Author

Yes I could look at a forked crate for now. Unfortunate that there will be multiple published gjson crates, but not the end of the world.

Thanks for letting me know.

@deankarn deankarn mentioned this pull request Apr 4, 2022
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