fix: compressor didn't chunkify big payload #3
Conversation
lib/compress-stream.js
Outdated
| @@ -52,18 +60,25 @@ CompressStream.prototype._uncompressed = function (chunk) { | |||
|
|
|||
| CompressStream.prototype._transform = function (chunk, enc, callback) { | |||
| var self = this | |||
| async function compressChunks() { | |||
There was a problem hiding this comment.
the API of transform is to call callback on the completion of transformation of the chunk, so don't want to holdup transform
There was a problem hiding this comment.
But there is no await in this body, and compressChunks() is called without any handling. Errors will become unhandled rejections. Can you just make it
function compressChunks() {
There was a problem hiding this comment.
I guess the callback can error and cause unhandled rejection.
Maybe the implementation can look like:
new Promise(() => {
...
callback(...);
...
}).catch((e) => logUnexpectedError(e);There was a problem hiding this comment.
done, just console logged as nothing fancy available in lib
| for (let startFrom = 0; startFrom < chunk.length; startFrom += UNCOMPRESSED_CHUNK_SIZE) { | ||
| const endAt = startFrom + Math.min(chunk.length - startFrom, UNCOMPRESSED_CHUNK_SIZE); | ||
| const bytesChunk = chunk.slice(startFrom, endAt); | ||
| const compressed = snappy.compressSync(bytesChunk) |
There was a problem hiding this comment.
Why switch to the sync version? If we want to do that we should do it in another PR as study in depth the implications
There was a problem hiding this comment.
@dapplion the async version where I chunk bytes from outside (like call compressStream.write(chunk)) which uses the original flow, compared to this where chunking happen in the transform using this way is 2X better slower
(i.e. compressSync based solution is coming out ahead):
(orig-snappy is the original with data chunked at compressStream.write level)
If I use async version inside with await Promise((resolve)=>... wrap to make sure the async returned data is written in a proper serial order, its 10-5% slower because of the overhead, if I don't care about the serial order in which chunks are written (which leads to incorrect stream), then async chunking version inside transform is 5% better.
I think for now this is our best bet 🙂
There was a problem hiding this comment.
Okay sounds good to me. Can you commit a benchmark tho? To inform us of the cost of compressing an uncompressing objects of 10Kb, 100Kb, 1000Kb
|
I suggest merging #4 first and reverting version bump here^^ |
mpetrunic
changed the title
lib/compress-stream.js
Outdated
| * | ||
| */ | ||
|
|
||
| CompressStream.prototype._transform = function(chunk, enc, callback) { | ||
| var self = this |
There was a problem hiding this comment.
I guess we don't need self anymore now
As per snappy frame encoding format, an uncompressed chunk can't be greater than
UNCOMPRESSED_CHUNK_SIZE = 65536.This PR updates the same in the compressor and includes a test case for the same, as well as bumps the version.