-
Notifications
You must be signed in to change notification settings - Fork 1.1k
perf: improve field indexing in JSON StructArrayDecoder (1.7x speed up) #9086
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
base: main
Are you sure you want to change the base?
Conversation
scovich
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understand the indexing code well enough to say whether that part is correct, but the idea of using an optional index for field name lookups makes a lot of sense to me.
| } | ||
| } | ||
|
|
||
| fn build_field_index(fields: &Fields) -> Option<HashMap<String, usize>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
qq: Do lifetimes coincide so that we could return Option<HashMap<&str, usize>> instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the lifetimes do coincide. we can use HashMap<&'a str, usize> by taking fields: &'a Fields as a parameter, which avoids the self-referential struct problem. However, this would require threading the lifetime parameter <'a> through the entire decoder system across many files. Since the lookup performance is identical, I don’t think it’s worth the added complexity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe it would be a good follow on PR
alamb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
arrow-json/benches/reader.rs
Outdated
| use std::fmt::Write; | ||
| use std::sync::Arc; | ||
|
|
||
| fn build_schema(field_count: usize) -> Arc<Schema> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you please add some comments here with an example of what this code does / what patterns of input it creates?
Also, it would help me to reproduce your results if you could make a separate PR with the benchmarks (so I can compare main to the PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
separate benchmark here
| } | ||
| } | ||
|
|
||
| fn build_field_index(fields: &Fields) -> Option<HashMap<String, usize>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe it would be a good follow on PR
|
run benchmark json-reader |
|
🤖 Hi @alamb, thanks for the request (#9086 (comment)).
Please choose one or more of these with |
…exing in StructArrayDecoder
…ecoders and field index creation
7e3077e to
06ded8b
Compare
|
run benchmark json-reader |
|
🤖 Hi @Weijun-H, thanks for the request (#9086 (comment)). |
|
🤖 |
|
🤖: Benchmark completed Details
|
alamb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Weijun-H -- I think this PR is a nice improvement. I have some suggestions on how to make it faster and improve the comments, but overall very nice 👍
| is_nullable: bool, | ||
| struct_mode: StructMode, | ||
| field_name_to_index: Option<HashMap<String, usize>>, | ||
| child_pos: Vec<u32>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a comment that explains what child_pos is? It isn't clear here (the idea of caching rather than recreating it looks good though)
Specifically I think it is important to document what is stored at each index (e.g. each index the tape position of at field_idx * row_count + row)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed and commented in df9e710
| )) | ||
| })?; | ||
| } | ||
| self.child_pos.resize(total_len, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like it would set some elements to zero twice -- I think you can get the same result without the extra setting via
self.child_pos.clear();
self.child_pos.resize(total_len, 0);Also, I think resize calls reserve internally (it internally calls extend_with which calls reserve), so there is no need to also call child_pos.reserve above
(also the rest of this crate just calls reserve so I think using try_reserve just here seems unecessary)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed in. df9e710
| fields.len() | ||
| ))); | ||
| } | ||
| child_pos[entry_idx * row_count + row] = cur_idx; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 this is a nice way to avoid allocations
…ecoder for improved clarity and performance
Which issue does this PR close?
Rationale for this change
Optimize JSON struct decoding on wide objects by reducing per-row allocations and repeated field lookups.
What changes are included in this PR?
Reuse a flat child-position buffer in
StructArrayDecoderand add an optional field-name index for object mode.Skip building the field-name index for list mode; add overflow/allocation checks.
Are these changes tested?
Yes
Are there any user-facing changes?
No