-
Notifications
You must be signed in to change notification settings - Fork 47
Require cached pref much lower #202
Comments
@mhdawson Can you post the script to run this 'require' test, please? I lost the screen content after leaving the zoom meeting today. |
Here it is: # run the require benchmark
$NODE_ROOT/deps/npm/bin/npm-cli.js install benchmark
export NODE_PATH=$WORKSPACE/bench/node_modules
REQUIRE_RESULT=`node ../benchmarking/experimental/benchmarks/node-api/require/require.perf.js 2`
echo $REQUIRE_RESULT
REQ_NEW_RESULT=`echo $REQUIRE_RESULT | awk 'BEGIN { FS = "," }; {print $2 }' | awk 'BEGIN { FS = " " }; {print $1 }'`
REQ_CACHED_RESULT=`echo $REQUIRE_RESULT | awk 'BEGIN { FS = "," }; {print $3 }'` |
@mhdawson Thanks. I'll work on this issue. |
Just created nodejs/build#1422 to give @uttampawar access. |
@mhdawson I was able to track this down and determined that this is not a regression. I don't think it's really a drop merely a reporting error. In fact for this test, we should report reduced "ops/sec" as a improvement. There are two numbers reported while running require_perf.js benchmark as shown below, Using master node branch. Anyway, following commit introduced this change nodejs/node@28f0693796. |
The chart label says ops/s so I think that is what we believed was being reported. For 6.x (and the previous versions before the change in the charts), the number was higher for require.cached which is what we expected. Looking at the runner however, I see: var sum = results.reduce(function(s, event) {
return s + event.target.hz;
}, 0);
var average = sum / results.length;
console.log([nameOfTest].concat(average).join(',')); Which does seem to indicate it is reporting the average time per run as opposed to ops/s |
But then the number fo 6.x is hard to understand in that it would say that it takes longer to load a cached module than a non-cached one. |
Looking at the code changed by nodejs/node@28f0693 unless adding to the children speeds up lookup, that change looks like it would only slow down require as opposed to speed it out. @uttampawar can you review the benchmark code to confirm what you believe it is measuring (as I'm not sure based on my quick look at the code and the change) and if we expect a speedup/slowdown from the change you referenced in terms of require for cached modules. |
Also looking at this in the test runner code
That seems to indicate it might be reporting hz (ops per second?) as we originally thought. |
@mhdawson I'll take a look. |
I think the units are |
@mhdawson Yes it looks that way, that change of updatingChildren when cached module is found is only going to slow down require. But simple test as shown below shows dramatic difference ... $ time node top.js $ cat top.js (without resetting the _cache) $ time top.js Huge difference. |
@mhdawson Here is the update. I debugged the code and it's definitely working as expected (as shown in previous example). $ node experimental/benchmarks/node-api/require/require.new perf.js 2 $ node experimental/benchmarks/node-api/require/require.cached perf.js 2 I not sure but it could be something to do with require.perf.js loading "a.js" in the main module. If you reverse the invocation of cached and the new we get following, |
Reported regression was due to underlying runner.runBenchmarks() function. Workaround is to revert the order of functions require.new and require.cache. Fixes: nodejs#202
Reported regression was due to underlying runner.runBenchmarks() function. Workaround is to revert the order of functions require.new and require.cache. PR-URL: nodejs/benchmarking#257 Fixes: nodejs/benchmarking#202 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
I just noticed this regressed (quite a while ago).
https://benchmarking.nodejs.org/charts/require_cached.png
Seems like its in the 3400 range on master, 8,x and canary and way higher on 6.x.
This may have been known, but would be doc to capture here what the reason was.
The text was updated successfully, but these errors were encountered: