-
Notifications
You must be signed in to change notification settings - Fork 2
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
fix: compressor didn't chunkify big payload #3
Changes from 7 commits
0757db6
6e22a09
ab41c6d
ce0a908
5c0a678
d6c2786
0e7e5ef
21c7b4c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,11 @@ | ||
/** | ||
* As per the snappy framing format for streams, the size of any uncompressed chunk can be | ||
* no longer than 65536 bytes. | ||
* | ||
* From: https://github.com/google/snappy/blob/main/framing_format.txt#L90:L92 | ||
*/ | ||
const UNCOMPRESSED_CHUNK_SIZE = 65536; | ||
|
||
var Transform = require('stream').Transform | ||
, util = require('util') | ||
|
||
|
@@ -50,20 +58,40 @@ CompressStream.prototype._uncompressed = function (chunk) { | |
) | ||
} | ||
|
||
CompressStream.prototype._transform = function (chunk, enc, callback) { | ||
/** | ||
* Some compression benchmarks : | ||
* | ||
* i) Chunking in transform with snappy.compressSync (the new implementation) | ||
* ii) Chunking from outside with compressStream.write (using original snappy.compress) | ||
* iii) No chunking (Original) | ||
* | ||
* | Size | in transform | compressStream.write | orginal (no chunking) | | ||
* |-------------------|--------------|----------------------|-----------------------| | ||
* | 10kb (1 chunk) | 0.0229 ms | 0.0385 ms | 0.0388 ms | | ||
* | 100kb (2 chunks) | 0.0562 ms | 0.1051 ms | 0.0844 ms | | ||
* | 1000kb (16 chunks)| 0.382 ms | 0.7971 ms | 0.1998 ms | | ||
* | ||
*/ | ||
|
||
CompressStream.prototype._transform = function(chunk, enc, callback) { | ||
var self = this | ||
|
||
snappy.compress(chunk, function (err, compressed) { | ||
if (err) | ||
return callback(err) | ||
|
||
if (compressed.length < chunk.length) | ||
self._compressed(chunk, compressed) | ||
else | ||
self._uncompressed(chunk) | ||
|
||
callback() | ||
}) | ||
new Promise(() => { | ||
try { | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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 (i.e. compressSync based solution is coming out ahead): If I use async version inside with I think for now this is our best bet 🙂 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
if (compressed.length < bytesChunk.length) | ||
self._compressed(bytesChunk, compressed) | ||
else | ||
self._uncompressed(bytesChunk) | ||
|
||
} | ||
callback(); | ||
} catch (err) { | ||
return callback(err); | ||
} | ||
}).catch(e => console.log(e)) | ||
} | ||
|
||
module.exports = CompressStream | ||
module.exports = CompressStream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we don't need self anymore now