benchmark: add bench for zlib gzip + gunzip cycle

[addaleax]

Originally wrote this for some work that is going to take a while
longer before it’s ready to be PR’ed, so it seems fine to start
with this on its own.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

[lpinca]

Nit: any reason for using fs.readFileSync(__filename) instead of an arbitrary fill character? Is it for making the chunks to gzip/gunzip realistic?

[addaleax]

@lpinca Yes, exactly that. If you have other suggestions, I’ll take them too :)

[lpinca]

Nah, that's ok and I have no better ideas.

[BridgeAR]

These options have to be reflected in the benchmark test as well (parallel/test-benchmark-zlib.js).

[lpinca]

@BridgeAR they are no? See line 14.

[apapirovski]

As far as I can tell, inputLen and duration have to be added to the test.

[lpinca]

Oh, true.

[lpinca]

Actually inputLen and duration are used in the test.

[apapirovski]

Right now we just have:

runBenchmark('zlib',
             [
               'method=deflate',
               'n=1',
               'options=true',
               'type=Deflate'
             ]);

[lpinca]

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 :)

[apapirovski]

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.

[lpinca]

I see now, I didn't read the original comment carefully. Sorry for the noise.

[addaleax]

Done!

[BridgeAR]

Should not better write be replaced instead of input.write?

[addaleax]

I’d find it a bit surprising to override something defined with function foo(){}, tbh…?

[BridgeAR]

Ping @addaleax

[BridgeAR]

Just to make sure it does not land as is.

[addaleax]

@BridgeAR PTAL

CI: https://ci.nodejs.org/job/node-test-pull-request/14552/

[addaleax]

CI: https://ci.nodejs.org/job/node-test-commit/18085/

[refack]

I'm -0 on this. It's heuristic (so still might fail on a really slow or clogged machine), probably safer to call bench.end(gb) with

// Give result in GBit/s, like the net benchmarks do.
// Never return 0, so that tests won't fail on really slow machines.
const gb = (readFromOutput * 8 / (1024 ** 3)) || 1

[refack]

ohh and #20128 - settings duration=1 will add a 1sec test to the suite.

[addaleax]

Do we do this elsewhere? Otherwise it seems like it would falsify test results – what if the test is actually broken and returns 0?

Would you be be okay with the current code if it passes a stress test run?

[addaleax]

Regarding the duration: Yeah, that’s not great. It’s not quite as bad as if it were a sequential test though…

[refack]

Let me read through the test and harness a little bit more. AFAIK the test-benchmark-* set are there "mostly" for syntax, API and regression testing. Designing correctness tests for the benchmarks might be out-of-scope.
Maybe we can just relax the Error: called end() with operation count <= 0 rule anyway 🤔

[refack]

Found it:

node/benchmark/common.js

Line 197 in 48e90ed

if (!process.env.NODEJS_BENCHMARK_ZERO_ALLOWED && operations <= 0) {

so calling:

runBenchmark('zlib',
             [
               'method=deflate',
               'n=1',
               'options=true',
               'type=Deflate',
               'inputLen=1024',
               'duration=0.001'
             ],
             {
               'NODEJS_BENCHMARK_ZERO_ALLOWED': 1
             });

should get the test to pass.

[addaleax]

Thanks, done!

[addaleax]

CI: https://ci.nodejs.org/job/node-test-commit/18087/

[apapirovski]

@addaleax I can't get this properly landed, it seems to be giving me a conflict. Any chance you can rebase? My bad. User error 😆

[apapirovski]

Landed in e797d5b