Relocate lifecycle logic from lib/index.js to lib/lifecycle.js so that it can run cleanly in the worker thread #139
Conversation
|
delay(50) returning in 49.02 ms is a known issue with Node.js, tracked down to an issue in libuv that IIRC is only fixed in Node 24. I participated in the report commentary. Let me see if I can hunt you up a ticket. These tests are not quite right due to that. github.com/libuv/libuv/issues/4773 |
RafaelGSS
left a comment
RafaelGSS
left a comment
There was a problem hiding this comment.
I don't get the reasoning behind this change.
You have a TODO about fixing tuning for workers in index.js. https://github.com/RafaelGSS/bench-node/blob/main/lib/index.js#L204 The only reason it didn’t already work is because all the code for tuning is in lifecycle.js, but is being driven entirely by index.js. worker-runner also calls lifecycle.js, but doesn’t run the same setup calls. It’s far simpler to push the setup code down than to duplicate identical logic in both places. This is a Code Smell that Refactoring (Fowler) labels “Feature Envy” - when any module or object is telling another one its own business. The solution is you move the responsibility for an action back where the action lives, or you split the two files into three and you move that responsibility out of both files into the third. But we already have three files involved and “lifecycle.js” is pretty on the nose. So it’s just a push down. |
index.js is doing a bunch of 'feature envy' on lifecycle.js and one of the consequences of that is that useWorkers doesn't necessarily get proper warmup runs in the worker thread. By pushing the logic into lifecycle::runBenchmarks, we can do the warming there. Special case for useWorkers=true getting a single benchmark in the receiving worker thread instead of the entire array. This should fix RafaelGSS#136
…ead. This data is already coming from the benchmark object.
jdmarshall
force-pushed
the
lifecycle
branch
from
6d4303a to
ec3eb24
Compare
|
This PR now has some significant tension with the DCE changes. I need to think a bit more about how to accomplish this. #136 still needs to be addressed, and I have designs on using it to improve faceoff. Both speed and to support benchmarking code that needs to run in isolation to get accurate results. |
2 participants
This looks like a case of Feature Envy emerging from iterative development. I think it's clear from where I sit now that this logic belongs in lifecycle.js, where it is a straight shot to run the lifecycle management code without any more special casing than wrapping
benchmarkin an array.This should address #136