Convert handles to hold weak reference to their executor #4624
Conversation
This file contains hidden or 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
Signed-off-by: Nicholas Gates <nick@nickgates.com>
Signed-off-by: Nicholas Gates <nick@nickgates.com>
gatesn
commented
Sep 13, 2025
| /// The future is provided a [`Handle`] to the runtime so that it may spawn additional tasks | ||
| /// to be executed concurrently. | ||
| fn block_on<Fut, R>(&self, f: Fut) -> R | ||
| fn block_on<F, Fut, R>(&self, f: F) -> R |
Contributor
Author
There was a problem hiding this comment.
We change the API to take a handle so it's harder to write code that drops the runtime too early.
It would be impossible to write code that drops the runtime too early if we could embed a lifetime in the handle. But alas. My week long dive into Rust revealed this is simply not possible :'(
gatesn
changed the title
Signed-off-by: Nicholas Gates <nick@nickgates.com>
CodSpeed Performance ReportMerging #4624 will not alter performanceComparing Summary
|
Codecov Report❌ Patch coverage is ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Nicholas Gates <nick@nickgates.com>
robert3005
approved these changes
Sep 13, 2025
Contributor
robert3005
left a comment
robert3005
left a comment
There was a problem hiding this comment.
the statics inside functions are a bit weird style but they work
gatesn
added a commit
that referenced
this pull request
Sep 15, 2025
This PR moves our I/O layer over from the old pre-fetching segment source to the new coalescing File I/O implementation on the Vortex runtime. Note: this PR currently includes #4624 ## Breaking Changes * `VortexOpenOptions` is no longer generic. * Fix: find/replace `VortexOpenOptions::file` -> `VortexOpenOptions::new` * `VortexOpenOptions::in_memory().open(...)` is replaced by `VortexOpenOptions::new().open_buffer(...)` ## Performance Generally this PR is about even, or slightly improved. There is some variance on some of the S3 benchmarks. The big regressions are in DuckDB on S3, where our read path has a custom work-stealing queue that doesn't buffer enough of the scan to perform effective pre-fetching. DataFusion doesn't have this problem, so we see much less of an impact. I want to follow up to this PR to switch over DuckDB to avoid this getting large, and accept the short-term regression. --------- Signed-off-by: Nicholas Gates <nick@nickgates.com>
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.
This fixes a problem where we can spawn a task that holds onto handle (since it has a static lifetime, ergh) can keep a runtime alive even after the caller drops it. For futures that use channels, e.g. the FileRead I/O, this results in read futures that hang forever when the runtime is dropped (because the executor holds the pending recv end of the channel, and the executor is kept alive by the send end of the channel...)
We also impl BlockingRuntime for CurrentThreadRuntime as this will likely become the default runtime for language bindings to use.