LibCompress(+AK+LibHTTP): Implement streamable asynchronous deflate and zlib decompression #24567
Conversation
github-actions
bot
added
the
👀 pr-needs-review
There was a problem hiding this comment.
Nothing looks particularly out of place (other than what I marked using inline comments), but effectively reviewing an entire second deflate implementation is somewhat hard.
What I'd be most interested in by now is actually seeing how existing things (like our existing Deflate and Zlib implementations) are gradually converted. We won't realistically be able to duplicate everything before starting to delete the non-async stuff.
| // These are defined just to replace some 4s and 8s with meaningful expressions. | ||
| using WordType = u32; | ||
| using DoubleWordType = u64; |
There was a problem hiding this comment.
Apart from the fact that Word and DoubleWord are Microsoft/Intel terminology that don't actually make sense in practice, why are we splitting here in the first place?
There was a problem hiding this comment.
No particular reason, these are defined just to replace some 4s and 8s with meaningful expressions. It is a bit nicer to think of reading in words and, therefore, chunks of sizeof(Word) bytes. As for DoubleWordType, I need to represent two consecutive words somewhere too.
In fact, BufferBitView works correctly if one replaces WordType with u64 and DoubleWordType with unsigned __int128 (I haven't done this only because of the performance and the fact that we only read at most 15 bits in deflate).
| auto ptr = reinterpret_cast<FlatPtr>(bytes.data()); | ||
| auto buffer_offset_in_bytes = ptr % alignof(WordType); | ||
| auto bytes_in_current_word_to_fill = sizeof(WordType) - buffer_offset_in_bytes; | ||
|
|
||
| m_bit_position = buffer_offset_in_bytes * 8 + bit_position; | ||
| m_bits_left = bytes.size() * 8 - bit_position; |
There was a problem hiding this comment.
Does aligned vs unaligned really make that much difference, especially since we just memcpy it over anyways?
There was a problem hiding this comment.
I was mostly thinking about sanitizer warnings and avoiding theoretical UB
| DeflateDecompressor::DeflateDecompressor(NonnullOwnPtr<AsyncInputStream>&& input) | ||
| DeflateDecompressor::DeflateDecompressor(MaybeOwned<AsyncInputStream>&& input) |
There was a problem hiding this comment.
Looks to have ended up in the wrong commit (same for the header file).
There was a problem hiding this comment.
Well, this was deliberate: non-owning inflate is not particularly useful by itself and is only used in zlib decompression. I can fix up this change, though, if you wish
DanShaders
added
⏳ pr-waiting-for-author
DanShaders
force-pushed
the
coro-decompress
branch
from
0177526
to
9692e29
Compare
github-actions
bot
added
👀 pr-needs-review
There is nearly not enough async-specific stuff in AsyncStreamBuffer for it to carry "Async" prefix.
DanShaders
force-pushed
the
coro-decompress
branch
from
9692e29
to
fadc4ae
Compare
DanShaders
force-pushed
the
coro-decompress
branch
from
fadc4ae
to
927c396
Compare
This PR builds upon previous work in the foundational coroutines PR and implements streamable asynchronous, error-safe, and EOF-correct decompression. Incidentally, new asynchronous implementation is about 2 times faster than our previous synchronous one.
In the future, to address code duplication the PR introduces, I plan to create a AsyncStream -> Stream translation mechanism and use it to reroute old classes to the new implementation.
(Deflate algorithm itself is pretty much directly copied from the old implementation, so it probably doesn't require as much attention as scaffolding.)
@timschumi, I'm sorry for yet another gigantic PR :).