-
Notifications
You must be signed in to change notification settings - Fork 30k
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
benchmark: add bench for zlib gzip + gunzip cycle #20034
Changes from 2 commits
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 |
---|---|---|
@@ -0,0 +1,39 @@ | ||
'use strict'; | ||
const common = require('../common.js'); | ||
const fs = require('fs'); | ||
const zlib = require('zlib'); | ||
|
||
const bench = common.createBenchmark(main, { | ||
inputLen: [1024], | ||
duration: [5], | ||
type: ['string', 'buffer'] | ||
}); | ||
|
||
function main({ inputLen, duration, type }) { | ||
const buffer = Buffer.alloc(inputLen, fs.readFileSync(__filename)); | ||
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. Nit: any reason for using 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. @lpinca Yes, exactly that. If you have other suggestions, I’ll take them too :) 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. Nah, that's ok and I have no better ideas. |
||
const chunk = type === 'buffer' ? buffer : buffer.toString('utf8'); | ||
|
||
const input = zlib.createGzip(); | ||
const output = zlib.createGunzip(); | ||
|
||
let readFromOutput = 0; | ||
input.pipe(output); | ||
if (type === 'string') | ||
output.setEncoding('utf8'); | ||
output.on('data', (chunk) => readFromOutput += chunk.length); | ||
|
||
function write() { | ||
input.write(chunk, write); | ||
} | ||
|
||
bench.start(); | ||
write(); | ||
|
||
setTimeout(() => { | ||
// Give result in GBit/s, like the net benchmarks do | ||
bench.end(readFromOutput * 8 / (1024 ** 3)); | ||
|
||
// Cut off writing the easy way. | ||
input.write = () => {}; | ||
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. Should not better 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. I’d find it a bit surprising to override something defined with |
||
}, duration * 1000); | ||
} |
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.
These options have to be reflected in the benchmark test as well (
parallel/test-benchmark-zlib.js
).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.
@BridgeAR they are no? See line 14.
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.
As far as I can tell,
inputLen
andduration
have to be added to the test.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.
Oh, true.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.
Actually
inputLen
andduration
are used in the test.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.
Right now we just have:
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.
It seems this benchmark is like net benchmarks where throughput is calculated after x sec. I don't think this will be run in CI. Or I am not understanding the issue :)
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.
Right but we have tests that run these benchmarks to confirm that they work and also to confirm that we didn't break some functionality. See
test/parallel/test-benchmark-zlib.js
.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 see now, I didn't read the original comment carefully. Sorry for the noise.
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.
Done!