Skip to content

Revert "Stop IO event stream once there are no more senders"#6209

Merged
gatesn merged 1 commit intodevelopfrom
revert-6203-adamg/event-stream
Jan 29, 2026
Merged

Revert "Stop IO event stream once there are no more senders"#6209
gatesn merged 1 commit intodevelopfrom
revert-6203-adamg/event-stream

Conversation

@gatesn
Copy link
Copy Markdown
Contributor

@gatesn gatesn commented Jan 29, 2026

Reverts #6203

I can believe we may in some cases process in-flight requests after the senders have been dropped, but it's not entirely clear to me that this PR actually implements that behavior since the coalescing driver eagerly pulls everything off this event stream, meaning any check would have to be inside the driver's poll_next function.

That said, it's still not obvious to me that the sender being dropped means that there's no one waiting for the result of the I/O request. I can open a file, submit a request, hold onto the request future and drop the file object.

@gatesn gatesn requested a review from AdamGS January 29, 2026 18:42
@gatesn gatesn enabled auto-merge (squash) January 29, 2026 18:42
@gatesn gatesn added the changelog/chore A trivial change label Jan 29, 2026
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented Jan 29, 2026

Merging this PR will degrade performance by 11.99%

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

⚡ 2 improved benchmarks
❌ 1 regressed benchmark
✅ 1176 untouched benchmarks
⏩ 1323 skipped benchmarks1

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
WallTime 1M_50pct[500000] 48.5 µs 55.1 µs -11.99%
WallTime 1M_90pct[1000000] 88.8 µs 60.4 µs +46.99%
WallTime u8_FoR[10M] 38.6 µs 6.6 µs ×5.9

Comparing revert-6203-adamg/event-stream (ec08bdd) with develop (a4a2f0d)

Open in CodSpeed

Footnotes

  1. 1323 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@AdamGS
Copy link
Copy Markdown
Contributor

AdamGS commented Jan 29, 2026

Just putting what we've discussed here for other people.

You can drop the file and hold on to the scan or other pieces, but the IoStream stays alive as long as the SegmentSource which is part of the layout reader. If we revert this change, that background thread will keep processing requests (anecdotally - ran tpch with one iteration, and in total there were 652 futures in the channel after all the work was done, and that's while we still cache layout readers).

There definitely seems to be more work on the IoRequestStream side, but I do think this is a step in the right direction.

@gatesn gatesn disabled auto-merge January 29, 2026 20:41
@gatesn gatesn merged commit 12078a9 into develop Jan 29, 2026
77 of 84 checks passed
@gatesn gatesn deleted the revert-6203-adamg/event-stream branch January 29, 2026 20:41
AdamGS pushed a commit that referenced this pull request Feb 2, 2026
Reverts #6203

I can believe we may in some cases process in-flight requests after the
senders have been dropped, but it's not entirely clear to me that this
PR actually implements that behavior since the coalescing driver eagerly
pulls everything off this event stream, meaning any check would have to
be inside the driver's poll_next function.

That said, it's still not obvious to me that the sender being dropped
means that there's no one waiting for the result of the I/O request. I
can open a file, submit a request, hold onto the request future and drop
the file object.
danking pushed a commit that referenced this pull request Feb 6, 2026
Reverts #6203

I can believe we may in some cases process in-flight requests after the
senders have been dropped, but it's not entirely clear to me that this
PR actually implements that behavior since the coalescing driver eagerly
pulls everything off this event stream, meaning any check would have to
be inside the driver's poll_next function.

That said, it's still not obvious to me that the sender being dropped
means that there's no one waiting for the result of the I/O request. I
can open a file, submit a request, hold onto the request future and drop
the file object.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog/chore A trivial change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants