Feature Request: Add a setup and teardown method

[jdmarshall]

Given the warnings in 'Managed Benchmarks', it seems like trying to do your own start() and end() is a bad plan. But in addition to this problem, it's not conducive to async testing, since for loops don't like await.

It would be much simpler if bench-node just supported setup and teardown functions like a number of other benchmarking tools provide.

[RafaelGSS]

Sorry, I didn't entirely get the issue you are describing. The idea behind bench-node is to always guarantee the code inside each benchmark function will run %DoNotOptimize to ensure no JIT-eliminations.

If I am understanding it correctly, your suggestion is to instead of:

suite.add('readFileSync', (timer) => {
  const randomFile = Date.now();
  const filePath = `./${randomFile}.txt`;
  writeFileSync(filePath, Math.random().toString());

  timer.start();
  for (let i = 0; i < timer.count; i++) {
    readFileSync(filePath, 'utf8');
  }
  timer.end(timer.count);

  rmSync(filePath);
});

We do

suite.add(
  'readFileSync',
  {
    setup: () => {
      const randomFile = Date.now();
      this.filePath = `./${randomFile}.txt`;
      writeFileSync(this.filePath, Math.random().toString());
    },
    teardown: () => {
      rmSync(this.filePath);
    }
  },
  () => {
    readFileSync(this.filePath, 'utf8');
  }
);

?

[jdmarshall]

Yes. That’s what tinybench and benchmark.js do. Unfortunately the latter is both retired and doesn’t handle async (you can make the test to be async but not the setup or teardown). And the former is bad about significant figures and comparisons. Nor does either have the nice charts.

The background is that I’m trying to build a replacement for benchmark-regression, after having worked on https://github.com/siimon/prom-client for a bit and way, way too long spent trying to trick it into handling async, cluster, or worker threads.

Btw do you need any help with your worker implementation? I may be of use.

[RafaelGSS]

PRs are always welcome.

The implementation of setup and teardown is not too much work - I can do that later today, possibly. But, async work does have influence of event loop ticks, so even if we change it to have a setup and teardown functions, if the main benchmark is async, this would not perform the same as sync functions, but that's realistic, so I guess it won't be a problem, right?

[jdmarshall]

I know you know a lot more about tricking v8 than I do. I suspect any solution I could provide would need corrections.

I don’t think anyone would be mad if v8 optimized the setup and teardown code. If you were trying to measure that, why would you put it in setup and teardown?

[H4ad]

Hey, some insights about setup and teardown, it was not included in the first release because of the https://github.com/RafaelGSS/bench-node/blob/main/lib/clock.js#L210, that´s why we created the managed benchmarks.

We used to serialize the function and then concatenate inside the actual script, with this change, we will break this little feature.

This feature of serializing is something that we got from the implementation of the benchmark.js

[RafaelGSS]

We can still serialize, but setup and teardown shouldn't reference any function/variable outside of their scope. We can copy the anonymous function.

[H4ad]

shouldn't reference any function/variable outside of their scope

It´s very likely people will use like that, unless we support keeping any new value assigned to this, to be able to reference/remove the created values in the setup, the type-safety is not great but works

[jdmarshall]

But benchmarkjs has setup and teardown?

I know benchmarkjs creates setup inside of the temp file it creates, I think it might for teardown as well. But I’m not convinced they needed to do that. Since all you need to do is pass in the results of the setup function into the generated code. It’s only run once and we don’t care how fast it is or isn’t.

It gets a little tricky with their defer support, which you sort of have with your timer. The callback is IIRC just always the last argument.

I filed a pr against benchmark-regression to fix its async support so you can see things like the test name and make decisions based upon it. Previously it would run setup before creating the benchmark. This took me half a day because the context is very narrow when calling into benchmarkjs. I tried several obvious solutions and got complaints about variables not being defined. I had to cheat a bit to get this to work. Benchmark-regression made it harder by using a transpiler which converts async-await to generator functions with yield, which was breaking scope, and this.

https://github.com/nowells/benchmark-regression/pull/3/files

Note that this also includes handling return false from the ‘start’ event to abort test scenarios that won’t work. I have my own solution for that, which doesn’t register the test in the first place. Other people might prefer a skip() method such as in mocha, which got it from rspec.

[jdmarshall]

We can still serialize, but setup and teardown shouldn't reference any function/variable outside of their scope. We can copy the anonymous function.

I think we were writing at the same time.

I need to be able to pass a value to setup, read back the answer, and pass it into both the benchmark and teardown. If you go this route I think I’ll have the same problem as with benchmark, and I don’t think you support the lifecycle events I used in the PR I mentioned above to trick it.

I think calling setup outside, and passing the returned value into the benchmark would be a better solution. One spread operator should do.

[jdmarshall]

bench-node/lib/worker-runner.js

Line 5 in 2ca0e80

function deserializeBenchmark(benchmark) {

This is likely to be the bigger problem. Passing data into workers is tricky.

[RafaelGSS]

The difference between conventional benchmark libraries and bench-node is that we encapsulate the benchmark function into a %DoNotOptimize function, so your code won't be JIT-eliminated, so it's not that simple.

We can have a benchmark-scoped this variable where reading and writing to the metadata is possible, but it's not a trivial task, especially guaranteeing this process will not affect the benchmark. I will try to think about a solution.

[jdmarshall]

I think if you solve it the way benchmark-regression does, by using an input parameter, that it can be made to work. I hope the PR I linked will have a couple ideas. I’m not entirely sure how my scope propagation to benchmarkjs is working there. That code is not easy to follow.

[jdmarshall]

This is the first thing I got to work:

https://github.com/cobblers-children/faceoff/tree/394dd1013bcf955dedb0034c5d243be5851612ab

I'm about to confuse this issue with additional functionality so that's a commit reference to the version I want to talk to you about.

And I don't understand why the 'module' parameter resolves even across the code generation you're doing. That's the same thing I mentioned above with benchmark.js. If I pass an object it works. Anything more complex, like a promise, and boom. My limited understanding is that this should not work either.

[RafaelGSS]

Thanks for the link, I'll check when I get some time