Faster Serde Integration (~80% faster) #4861
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tustvold
commented
Sep 26, 2023
| match i64::try_from(v) { | ||
| Ok(v) => self.serialize_i64(v), | ||
| Err(_) => { | ||
| let mut buffer = [0_u8; u64::FORMATTED_SIZE]; |
There was a problem hiding this comment.
The additional complexity to support rountripping values u64::MAX > v > i64::MAX seemed not worth it, so we just fallback to serializing to a string
There was a problem hiding this comment.
Would you imagine doing it via i128 or something?
There was a problem hiding this comment.
Possibly, or as a u64 variant. Given JSON only reliably roundtrips f64, I don't think this is a very common use-case worth optimising for
alamb
approved these changes
Sep 26, 2023
| /// Formatting to a string only to parse it back again is rather wasteful, | ||
| /// it may be possible to tweak the tape representation to avoid this | ||
| /// | ||
| /// Need to use macro as const generic expressions are unstable |
| match i64::try_from(v) { | ||
| Ok(v) => self.serialize_i64(v), | ||
| Err(_) => { | ||
| let mut buffer = [0_u8; u64::FORMATTED_SIZE]; |
There was a problem hiding this comment.
Would you imagine doing it via i128 or something?
| @@ -54,6 +55,25 @@ pub enum TapeElement { | |||
| /// | |||
| /// Contains the offset into the [`Tape`] string data | |||
| Number(u32), | |||
|
|
|||
| /// The high bits of a i64 | |||
| TapeElement::StartList(end_idx) => Ok(end_idx + 1), | ||
| TapeElement::StartObject(end_idx) => Ok(end_idx + 1), | ||
| _ => Err(self.error(cur_idx, expected)), | ||
| TapeElement::EndObject(_) | TapeElement::EndList(_) => { |
There was a problem hiding this comment.
+1 for removing the catch all
ryanaston
pushed a commit
to segmentio/arrow-rs
that referenced
this pull request
Nov 6, 2023
* Store decoded numerics in JSON tape * Add arrow-json serde benchmarks * Fix timestamp serialize * Clippy
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Closes #.
Rationale for this change
Encoding numerics directly in the tape drastically improves the performance of the serde integration.
It additionally opens the door to eager parsing in the future, which may yield performance improvements for regular JSON decoding.
I have confirmed this does not regress the performance of the JSON decoder
What changes are included in this PR?
Are there any user-facing changes?