Add the length field for Buffer and use more Buffer in IPC reader to avoid memory copy.
#2557
Conversation
Signed-off-by: remzi <[email protected]>
Signed-off-by: remzi <[email protected]>
Signed-off-by: remzi <[email protected]>
github-actions
bot
added
arrow
| @@ -354,7 +355,7 @@ async fn save_uploaded_chunks( | |||
|
|
|||
| let batch = record_batch_from_message( | |||
| message, | |||
| &data.data_body, | |||
| &Buffer::from(data.data_body), | |||
There was a problem hiding this comment.
I don't find a way to avoid memory copy here. (as I'm not familiar with arrow-flight)
There was a problem hiding this comment.
We would likely need to move to using https://docs.rs/bytes/latest/bytes/ to avoid this
There was a problem hiding this comment.
We would likely need to move to using https://docs.rs/bytes/latest/bytes/ to avoid this.
Hi @tustvold. Could you please file an issue to track it, because you might know more context about it ?
There was a problem hiding this comment.
Buffer::from_custom_allocation could avoid the copy.
There was a problem hiding this comment.
How does that API handle alignment?
Edit: I seem to remember something in the IPC format about buffer alignment, so perhaps this would be fine 🤔
There was a problem hiding this comment.
Right, forgot about alignment. That would only work if the ipc buffer is properly aligned for the type. I don't know what the requirements about that are, so the suggestion might be a bit fragile, or would need to check the alignment and do a copy as a fallback.
HaoYang670
changed the title
length field for Buffer and use more Buffer in IPC reader.length field for Buffer and use more Buffer in IPC reader to avoid memory copy.
|
Benchmark runs are scheduled for baseline = 3430537 and contender = ad84176. ad84176 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Closes #2524.
Related to #2437.
Rationale for this change
I put 2 things (Adding
lengthfield and updating IPC reader) together in this PR because I want to (informally) prove thatadding
lengthfield is valuable.Besides, I don't lift the length out of Bytes, although @tustvold suggested it, because it will cause more API changes. We could do that in following PRs.
What changes are included in this PR?
Are there any user-facing changes?
Yes.