Put BufWriter into TrackedWrite #3361
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
viirya
commented
Dec 17, 2022
Comment on lines
+318
to
+319
| let path = env::temp_dir().join("arrow_writer.temp"); | ||
| let file = File::create(path).unwrap(); |
There was a problem hiding this comment.
I changed it to write batch to file.
tustvold
reviewed
Dec 18, 2022
| @@ -45,16 +45,17 @@ use crate::schema::types::{ | |||
|
|
|||
| /// A wrapper around a [`Write`] that keeps track of the number | |||
| /// of bytes that have been written | |||
| pub struct TrackedWrite<W> { | |||
| inner: W, | |||
| pub struct TrackedWrite<W: Write> { | |||
There was a problem hiding this comment.
Perhaps we could also update the doc comment to mention the addition of a BufWriter
tustvold
approved these changes
Dec 18, 2022
There was a problem hiding this comment.
When writing to memory, does the additional BufWriter have an appreciable performance cost? That would be my only concern, otherwise this seems like a substantial win 👍
I also double-checked that large writes, such as when writing an entire page, will skip the buffer - https://doc.rust-lang.org/src/std/io/buffered/bufwriter.rs.html#364
Yea, on the benchmark of writing to in-memory writer, the additional BufWriter is slower. However I think file writer is preferred as this is for Parquet writer so I assume the practice usage is file-based instead of in-memory which is basically for test purpose. |
|
IOx writes to memory prior to flushing to object storage... How bad is the regression? |
|
For no-bloom filter ones, the regression is slightly less than 6.9%, but for bool types, it is 50~143% improvement. For bloom filter ones, 54% (string non-null with bloom filter) regression is worst one. |
|
Updated: By implementing some functions of Still regression ~25%: string bloom filter, string dictionary with bloom filter, string non-null with bloom filter. |
tustvold
approved these changes
Dec 19, 2022
|
Benchmark runs are scheduled for baseline = 2cf4abb and contender = e2abb4b. e2abb4b is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Closed
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 #3366.
Rationale for this change
To improve writing performance.
What changes are included in this PR?
Are there any user-facing changes?