fix: Benchmarks that use timers cannot use workers. #137
Conversation
jdmarshall
force-pushed
the
asyncWorker
branch
from
e191c42 to
4614cfe
Compare
lib/worker-runner.js
Outdated
| // Deserialize the benchmark function | ||
| function deserializeBenchmark(benchmark) { | ||
| benchmark.fn = new Function(benchmark.fn); | ||
| benchmark.fn = new AsyncFunction("timer", benchmark.fn); |
There was a problem hiding this comment.
In my research, adding unnecessary await async to a sync function reduces the performance and increases the variability of the benchmarked function by a significant amount.
Can you please test with and without your patch the ops/sec of examples/worker-threads/node.js?
There was a problem hiding this comment.
Well you’re stripping off the function signature on the sending side, so there’s no clean was to know unless we inspect it there first.
Found this while looking at another PR:
https://github.com/RafaelGSS/bench-node/blob/main/lib/index.js#L74
this.isAsync = types.isAsyncFunction(this.fn);
I’ll see what I make of that.
There was a problem hiding this comment.
I found a block of code in clock.js that does similar logic. I've added both an async and a hasArg check which should retain as much throughput as can be maintained. Unfortunately my library will hit both unless I do some more special casing in the function template which I really do not like.
Emulate the same optimizations from clock.js in worker-runner.js. This will only create an async function if the origin function is async, and will only trigger the Timer logic flow if the origin function contained the same.
2 participants
Fixes #135