Skip to content

concurrent compression and IO for segment writer #3182

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

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

onursatici
Copy link
Contributor

This will only have an effect on remote storage. Unfortunately we don't have those on our benchmarks yet

Ok(layout)
};
// compress chunks and flush concurrently, former is cpu bound latter is io.
let (layout, (mut write, segment_specs)) = future::try_join(write_chunks, flush).await?;
Copy link
Contributor

@0ax1 0ax1 Apr 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that compressing chunks is CPU bound, is a concurrent wait for write_chunks, flush the right mechanism opposed to putting the CPU bound work on a diff thread? Say a task is currently busy compressing, this also means that no progress can be made on issuing writes, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea this assumes we don't block that much while polling compression. My thinking was that compressing an average chunk would be shorter compared to writing the previous compressed chunk into remote storage, so the combined future would have some await points during the write.

Spawning here would be better and I have to add that while doing the next thing I plan to do (multithreaded compression), but that requires changing the VortexWrite a bit, as to spawn we need it to be Send + 'static

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