Skip to content

Conversation

@uazu
Copy link
Contributor

@uazu uazu commented Dec 3, 2025

I needed much finer control over the flush behaviour because I am
packet-streaming at low bandwidth and every byte counts. Typically I
need to scan ahead and check how much compressed data is available to
send without committing to any flush or sync at that point (so I need
clone), and then once downstream packet-handling code has requested
data, avoid committing to any sync until I'm sure I need it.

This change may result in smaller compressed output for some existing
code using the existing API (since an empty block generated by a flush
call with no data buffered will be skipped, unless it is required to
carry the "finish" flag).

I've tested on Rust stable and 1.60.

Jim Peters added 2 commits December 3, 2025 12:56
- Purpose is provisional flushing: clone, flush to measure output,
  then drop and revert to the original instance before the flush
- Make the code not output a block if there is no data to output
- Add new flush modes that don't add a sync sequence if the stream is
  already on a byte boundary
- Add a "no-sync" flush mode that flushes without a sync
- Test correct behaviour of all flush modes
@oyvindln
Copy link
Collaborator

oyvindln commented Dec 3, 2025

This looks fine, though adding new variants to TDEFLFlush is technically an API break since it's not tagged with non_exhaustive so it would require a minor version bump (even though I'm not super stoked about it given that last time I did it it resulted in projects not bothering to take the time to upgrade the dependency and being stuck with an older version where the compression is performance is broken due to a rust compiler regression...)

Given that I think I want to try to get a new release out with the current pending changes before merging this, and maybe also look into whether there is any other minor abi changes that are worth looking at while we're at it if we have to bump it anyhow.

@oyvindln
Copy link
Collaborator

oyvindln commented Dec 3, 2025

Actually scrap that it would probably be better to pair the minor version bump if needed with the current unreleased changes since that would mean any flate2 updates would require the new version and thus require users to update.

@uazu
Copy link
Contributor Author

uazu commented Dec 3, 2025

Thanks. I'm happy to wait and/or update the PR as necessary to fit your release schedule

@oyvindln oyvindln merged commit 0d276ac into Frommi:master Dec 8, 2025
10 checks passed
@oyvindln
Copy link
Collaborator

oyvindln commented Dec 8, 2025

Is it possible to re-work the test to be a bit more simple? It takes waaaay to long to run right now?

@uazu uazu deleted the flush-improvements branch December 8, 2025 20:36
@uazu
Copy link
Contributor Author

uazu commented Dec 8, 2025

I'll have a look at making the test run quicker tomorrow

@uazu
Copy link
Contributor Author

uazu commented Dec 9, 2025

Regarding the test slowness, it is a bad interaction with the other recent change "simplify stored compression". Either I'm using the API wrong, or that change introduced a regression.

In detail: The test compression that the flush-test does appears to be storing rather than compressing. Partial output length is input length +6. Data is very compressible so this shouldn't happen.

Previously, it was completing the test in 59 iterations. Now I'm way past 80K iterations and still it is only doing a "store". If I revert the stored compression patch, I'm back to 59 iterations.

I will change the flush test to bail out and fail earlier in this case.

@oyvindln
Copy link
Collaborator

oyvindln commented Dec 9, 2025

Regarding the test slowness, it is a bad interaction with the other recent change "simplify stored compression". Either I'm using the API wrong, or that change introduced a regression.

In detail: The test compression that the flush-test does appears to be storing rather than compressing. Partial output length is input length +6. Data is very compressible so this shouldn't happen.

Previously, it was completing the test in 59 iterations. Now I'm way past 80K iterations and still it is only doing a "store". If I revert the stored compression patch, I'm back to 59 iterations.

I will change the flush test to bail out and fail earlier in this case.

Hm, must be me messing up then and the test not catching it, will have to look into it

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.

3 participants