benchmark: try building the addons in napi benchmarks

[joyeecheung]

cc @Trott @refack
Refs: nodejs/build#1568

Checklist

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

[nodejs-github-bot]

@joyeecheung build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1579/pipeline

[refack]

So I'm not sure about this, since it's unconditional... Doing it in make gives us free dependency tracking (although it excludes Windows...)

[joyeecheung]

So I'm not sure about this, since it's unconditional... Doing it in make gives us free dependency tracking (although it excludes Windows...)

What do you mean by free dependency tracking? It's only run in the daily master CI so it's sort of conditional (also as far as the test is concerned, it skips Windows and Debug builds)

[Trott]

Would this change mean that bench-addons-build in Makefile is unnecessary and can be removed from that file as part of this PR?

Also, should this PR remove the isWindows guard/skip code in test-benchmark-napi?

[refack]

What do you mean by free dependency tracking? It's only run in the daily master CI so it's sort of conditional (also as far as the test is concerned, it skips Windows and Debug builds)

For CI your are right. But if I think about regular dev flow, I'd rather we build this IFF it's dependencies change:

node/Makefile

Lines 1051 to 1054 in e854cf9

	# Build required addons for benchmark before running it.
	.PHONY: bench-addons-build
	bench-addons-build: benchmark/napi/function_call/build/Release/binding.node \
	benchmark/napi/function_args/build/Release/binding.node

[Trott]

I'm totally 👍 on this approach or the Makefile approach in #24307. I'll form a stronger opinion if we don't have consensus, but otherwise I'll defer to the more-build-and-automation-informed folks on this stuff.

[refack]

Also this packs a surprise side effect, if I run node benchmark/napi/function_call I don't expect node-gyp to run (esp. on Windows were there is a high chance the system isn't set up for that to succeed).
Personally I'd rather it would bark at me to run it manually.

[joyeecheung]

For CI your are right. But if I think about regular dev flow, I'd rather we build this IFF it's dependencies change:

This will only build if the module failed to load - which usually means the dependency changes. But benchmark isn't in regular dev flow anymore since it's ignored by test.py?

If I run node benchmark/napi/function_call I don't expect node-gyp to run (esp. on Windows were there is a high chance the system isn't set up for that to succeed).

It doesn't run always though, only when the addon fail to load. If the addon fail to load, and node-gyp fail to run, the benchmark will exit as usual - only difference is that by giving it another try, node-gyp could do some damage to your system somehow? (But I don't see how?)

(Also just curious, are these benchmarks runnable on Windows?)

[refack]

This will only build if the module failed to load

Yes you are right. So yes that's a very good condition :)

only difference is that by giving it another try, node-gyp could do some damage to your system somehow? (But I don't see how?)

I'm thinking about this... ATM I'm not blocking, just not approving ¯_(ツ)_/¯

[joyeecheung]

@refack Are the benchmark ever run on Windows? They are skipped in tests for Windows, and I couldn't find a shortcut in vcbuild.bat so I don't assume people run them manually?

[refack]

I don't assume people run them manually?

Yeah, when you want to run benchmarks. Personally I call them directly or as described in https://github.com/nodejs/node/blob/master/doc/guides/writing-and-running-benchmarks.md

(AFAIK the calls from test/benchmark/* are just sanity tests. After all we discard the results).

[BridgeAR]

As a suggestion: I would rewrite this whole part to something like:

const v8 = common.buildAddon(
  `./build/${common.buildType}/binding`,
  'benchmark/napi/function_args'
);

In that case the common call just tries to require it first and returns it if possible and tries to build the addon otherwise.
If it fails, we also already have all the necessary information printed due to the console.error part in the common part and in the actual error that is thrown if it fails.

This would reduce the number of code lines significantly.

[jasnell]

Ping @joyeecheung ... is this still needed?

[jasnell]

Closing due to lack of continued activity. Can reopen if it is picked back up again