-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Add exposed window buffer mode #3017
base: dev
Are you sure you want to change the base?
Conversation
This is an interesting feature. I haven't looked at the code yet, but I don't see anything obviously wrong with the API you're describing. I like the requirement to acknowledge that you've consumed the buffer by setting it to 0. I'm going to want to think about the API implications a bit with @Cyan4973, but I think this feature has a strong chance of being accepted. |
This is a really interesting idea, and it would be great to give users a tool to avoid this copy. My fundamental concern is that the So this is probably not a solution we want to ship. But I would very much be open to discussing other possible solutions. |
Why is this a problem? It's an advanced option to be enabled by someone who wants this feature and knows the implications. But ok, if there's a better solution please go for it. Pros:
Cons:
Thoughts? |
It's certainly something we could disclaim in documentation, but I would very strongly prefer not to have the code lie about its contract with the user. Most people don't read the documentation, and the code itself needs as much as possible to communicate and enforce expectations about its use. A new API is probably the right way to proceed. Especially since we also need some way to coordinate with zstd how much of the buffer has been consumed. I guess in this existing mode, it's implicit that the user consumes the whole window that's being returned, but I don't see that the code actually updates what will be returned on subsequent calls based on that. |
Related to and discussed in #3004
This change renames
ZSTD_d_stableOutBuffer
toZSTD_d_outBufferMode
and adds a 3rd setting:The change is ABI-compatible.
Temporary source compatibility could be done by re-adding the original
to zstd.h until the parameter is no longer considered experimental.
--- What this does ---
ZSTD_bufmode_expose
gives a user direct read access into the LZ-window instead of flushing it with a memcpy.Instead of fiddling with an external buffer like the usual code:
we can now do this, completely avoiding an extra copy:
This is very nice for a streaming scenario where the consumer does not care about the exact size of the decompressed data and where the buffer is located in memory, as described in the ryg blog.
I guess avoiding the extra copy should also cause less cache thrashing but i didn't try to measure that.
Things left to discuss:
ZSTD_bufmode_expose
is currently only implemented for decompression.Since compression is quite resource-hungry this would not benefit from an exposed window. But it'd be nice to have the same API available for compression as well.
What about renaming
ZSTD_c_stable{In|Out}Buffer
toZSTD_c_outBufferMode
for consistency? (Didn't touch that so far because i wanted to avoid cluttering up the PR more that it is already.)Anything else left to do?
EDIT: Except for the C90 compat. Clang and vs19 are happy so i didn't see this. Oh well.
EDIT2: Fixed derp in the external buffer example block above, that was supposed to fwrite using pos, not size. In the exposed buffer block below, pos is always == 0. I considered setting output pos=size to ease a transition to this type of buffer handling but it caused some annoying side effects in the code that checks/handles pos so i didn't go there (mainly the pos<=size invariant, didn't want to touch that)
(cc @terrelln)