benchmark: build the misc/function_call addon

[joyeecheung]

• Build the addon required by benchmark/misc/function_call
• Throw errors in the addon if the binding could not be loaded

Checklist

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

Affected core subsystem(s)

test, benchmark, build

This patch builds the addon for benchmark/misc/function_call in build files when running tests, and throw an error in the benchmark when the addon is not built instead of failing silently.

[refack]

I'm not sure why, but the call above (L416) uses --nodedir="%cd%", so I'd rather we keep this consistent.
IMHO simplest solution is to add benchmark\misc\function_call to L413:

for /d %%F in (test\addons\* benchmark\misc\function_call) do (

[richardlau]

LGTM but I would prefer @refack's suggestion.

[joyeecheung]

Amended to use the suggestion in #16934 (comment)

New CI: https://ci.nodejs.org/job/node-test-pull-request/11353/

[joyeecheung]

Forgot the cross-compilation on arms so the tests failed there. Updated, but probably will need to wait for them to get back before starting another CI..

[joyeecheung]

Wait...all the bench-* targets seem to depend on all. Are those targets even necessary? Is there anyone running them alone? I think we just need to keep bench, bench-all and bench-ci? cc @nodejs/performance @AndreasMadsen

[AndreasMadsen]

It should be possible to run the benchmark suite without running make all. In theory, you should be able to use any semi-recent node version.

[joyeecheung]

@AndreasMadsen So IIUC, it is safe to remove those targets and make bench, bench-all and bench-ci independent of all? (use the local build if available, otherwise use the global node on PATH?)

[AndreasMadsen]

Yes. But we might have to update the CI benchmarker script.

[joyeecheung]

@AndreasMadsen OK thanks, I will take a look in the benchmark CI script.

[refack]

@joyeecheung FYI it's at https://github.com/nodejs/benchmarking/blob/master/experimental/benchmarks/community-benchmark/run.sh

[joyeecheung]

@refack Thanks, looks like it does not even use the bench-* targets but just call compare.js instead? So I think it's OK to remove those.

[refack]

So I think it's OK to remove those.

I'm +1 for GC on Makefile (as I commented, it is IMHO too complex)

[gareth-ellis]

@joyeecheung Yep, the benchmarking jobs just do a normal make, so as long as that doesn't disappear, we'll be fine :)

[AndreasMadsen]

@gareth-ellis But I suppose we should run make benchmark/misc/function_call/build/Release/binding.node in the CI benchmarker somehow?

[joyeecheung]

New CI: https://ci.nodejs.org/job/node-test-pull-request/11475/

[joyeecheung]

@refack @jasnell @richardlau Can you take a look again? Thanks

[jasnell]

still lgtm

[joyeecheung]

CI failed due to the makefile regression. Trying again: https://ci.nodejs.org/job/node-test-pull-request/11492/

[addaleax]

@joyeecheung This would need a rebase :)

[joyeecheung]

Rebased. CI: https://ci.nodejs.org/job/node-test-pull-request/11526/

[addaleax]

New CI: https://ci.nodejs.org/job/node-test-commit/14145/

[refack]

Fails on arm & shared-lib-debug:
https://ci.nodejs.org/job/node-test-commit-linux-linked/nodes=ubuntu1604_sharedlibs_debug_x64/166/console
https://ci.nodejs.org/job/node-test-binary-arm/11925/

09:04:26 not ok 22 parallel/test-benchmark-misc
09:04:26   ---
09:04:26   duration_ms: 14.912
09:04:26   severity: fail
09:04:26   stack: |-
09:04:26     
09:04:26     misc/console.js
09:04:26     misc/console.js n=1 concat=0 method="": 26.737569710525968
09:04:26     
09:04:26     misc/freelist.js
09:04:26     misc/freelist.js n=1: 57.44223293124154
09:04:26     
09:04:26     misc/function_call
09:04:26     /home/iojs/build/workspace/node-test-binary-arm/benchmark/misc/function_call/index.js:17
09:04:26       throw new Error('misc/function_call.js Binding failed to load');
09:04:26       ^
09:04:26     
09:04:26     Error: misc/function_call.js Binding failed to load

[refack]

Probably needs to be added to test-ci-js since the benchmark is tested from JS. I'd suggest adding a comment since this dependency is not intuitive.

[refack]

IMHO we should consider a different approach, since this is getting a bit convoluted (test => benchmark => native-addon => node-gyp => node binary).

• parallel/test-benchmark-misc should call node-gyp since for it to run the node binary must have been built.
• benchmark/misc/function_call should print message and exit(0) if can't require the addon

[joyeecheung]

@refack Yes that seems to be a better idea, I'll try running node-gyp from the test. Although I think it should not silently fail when there is node-gyp and the local node is executable. If these two are true, then not being able to build the addon means there is a real issue that needs to be addressed.

[joyeecheung]

Couldn't get gyp running from a Node.js script with the proper --xx="yy" arguments, there are some weird quote quirks and gyp keeps messing up the -I argument for common.gypi (always end up with '/Users/joyee/projects/node/"/Users/joyee/projects/node/"/common.gypi', the command threw by child_process works if run from the shell, but I cannot get gyp running like that when invoking from Node for the life of me)...I'll close this until I somehow figure out how to get gyp running from Node (if I can ever figure that out..)