Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: add test-benchmark-napi #23585

Closed
wants to merge 7 commits into from

Conversation

forivall
Copy link
Contributor

also makes sure that the napi benchmark is built before running jstest

skipped on windows since n-api benchmarks aren't built there yet

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

also makes sure that the napi benchmark is built before running jstest

skipped on windows since n-api benchmarks aren't built there yet
@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Oct 12, 2018
@targos targos added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Oct 12, 2018
@addaleax
Copy link
Member

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 12, 2018
const common = require('../common');

if (common.isWindows) {
common.skip('vcbuild.bat doesn\'t build the n-api benchmarks yet')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The linter fails on this due to the missing semicolon at the end.

@Trott
Copy link
Member

Trott commented Oct 13, 2018

You can run make lint-js to lint the JS files.

The new test is failing because it is running too many benchmark variants. I could explain in a GitHub comment, but we're in the same room right now, so I'll walk over.... Whoops, the test is failing because the Makefile task is probably not being run in CI...

@Trott
Copy link
Member

Trott commented Oct 13, 2018

@gireeshpunathil
Copy link
Member

open question: In other benchmark triggering tests we have seen checking for and skipping based on available memory - is this test an exemption?

@forivall
Copy link
Contributor Author

forivall commented Oct 13, 2018

@gireeshpunathil I noticed that the memcheck wasn't in all of the benchmark tests -- only the http and tls ones. buffer, child-process, dgram, net and path all do not have the memory restriction.
my intuition is that n-api was closest to buffer or child process

@Trott
Copy link
Member

Trott commented Oct 13, 2018

@Trott
Copy link
Member

Trott commented Oct 14, 2018

@richardlau
Copy link
Member

Failures are related (at least one of the n-api benchmark addons is failing to compile):
https://ci.nodejs.org/job/node-test-commit-linux/nodes=debian8-64/22354/console

23:43:23 ../napi_binding.c: In function ‘CallWithArray’:
23:43:23 ../napi_binding.c:53:5: error: ‘for’ loop initial declarations are only allowed in C99 or C11 mode
23:43:23      for (uint32_t i = 0; i < length; ++i) {
23:43:23      ^
23:43:23 ../napi_binding.c:53:5: note: use option -std=c99, -std=gnu99, -std=c11 or -std=gnu11 to compile your code
23:43:23 ../napi_binding.c: In function ‘CallWithArguments’:
23:43:23 ../napi_binding.c:176:5: error: ‘for’ loop initial declarations are only allowed in C99 or C11 mode
23:43:23      for (uint32_t i = 1; i < loop; ++i) {
23:43:23      ^
23:43:23 napi_binding.target.mk:99: recipe for target 'Release/obj.target/napi_binding/napi_binding.o' failed
23:43:23 make[2]: *** [Release/obj.target/napi_binding/napi_binding.o] Error 1
23:43:23 make[2]: *** Waiting for unfinished jobs....

We do set -std=gnu++1y for 'cflags_cc' in

'cflags_cc': [ '-fno-rtti', '-fno-exceptions', '-std=gnu++1y' ],

but I think this only applies to .cc files (napi_binding.c is a .c).

cc @nodejs/addon-api @nodejs/gyp

@richardlau
Copy link
Member

Sorry, meant to cc @nodejs/n-api instead of addon-api.

@NickNaso
Copy link
Member

Hi @richardlau,
maybe adding the following option "-std=c11" to the CFLAGS could solve the problem.
Now the options for CFLAGS are:

'cflags': [ '-Wall', '-Wextra', '-Wno-unused-parameter', ]

change in

'cflags': [ '-Wall', '-Wextra', '-Wno-unused-parameter', '-std=c11' ]

See:

'cflags': [ '-Wall', '-Wextra', '-Wno-unused-parameter', ],

@BridgeAR BridgeAR removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 15, 2018
@Trott
Copy link
Member

Trott commented Oct 17, 2018

common.gypi Outdated
@@ -369,7 +369,7 @@
'ldflags': [ '-pthread' ],
}],
[ 'OS in "linux freebsd openbsd solaris android aix cloudabi"', {
'cflags': [ '-Wall', '-Wextra', '-Wno-unused-parameter', ],
'cflags': [ '-Wall', '-Wextra', '-Wno-unused-parameter', '-std=c11', ],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since common.gypi is used by addons we should at least run this past citgm.

Copy link
Member

@Trott Trott Oct 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose an alternative is to hard-code the flag into the Makefile for the napi benchmark build step only?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe in the binding.gyp of the n-api benchmark addon?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for CITGM. It's failing to build on lots of hosts. Will force-push that commit out....

@Trott Trott force-pushed the forivall/test-benchmark-napi branch from 0e67357 to 8309f44 Compare October 17, 2018 04:46
@Trott
Copy link
Member

Trott commented Oct 17, 2018

Rewrote the loops to declare the variables before using in for.

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

@Trott
Copy link
Member

Trott commented Oct 19, 2018

Added common.skip() for workers.

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

@Trott
Copy link
Member

Trott commented Oct 20, 2018

For ubuntu1604_sharedlibs_debug_x64, do we need to skip the test or is there a bug to be fixed?

https://ci.nodejs.org/job/node-test-commit-linux-containered/7955/nodes=ubuntu1604_sharedlibs_debug_x64/console

22:29:34 not ok 2090 sequential/test-benchmark-napi
22:29:34   ---
22:29:34   duration_ms: 15.590
22:29:34   severity: fail
22:29:34   exitcode: 1
22:29:34   stack: |-
22:29:34     /home/iojs/build/workspace/node-test-commit-linux-containered/nodes/ubuntu1604_sharedlibs_debug_x64/benchmark/napi/function_args/index.js: V8 Binding failed to load
22:29:34     misc/function_call.js Binding failed to load
22:29:34     assert.js:351
22:29:34         throw err;
22:29:34         ^
22:29:34     
22:29:34     AssertionError [ERR_ASSERTION]: benchmark file not running exactly one configuration in test: 
22:29:34     napi/function_args
22:29:34     
22:29:34     napi/function_call
22:29:34     
22:29:34         at ChildProcess.child.on (/home/iojs/build/workspace/node-test-commit-linux-containered/nodes/ubuntu1604_sharedlibs_debug_x64/test/common/benchmark.js:43:12)
22:29:34         at ChildProcess.emit (events.js:182:13)
22:29:34         at Process.ChildProcess._handle.onexit (internal/child_process.js:240:12)
22:29:34   ...

@richardlau
Copy link
Member

The job config for ubuntu1604_sharedlibs_debug_x64 only runs build-ci -- it manually invokes test.py rather than any of the test-* targets (the other nodes in the job run run-ci). It therefore does not exercise the makefile changes in this PR and does not build the addons required to run the benchmark.

There may also be an issue in the benchmark as it is requiring from build/Release which is probably not what we want in a debug build (assuming the addon was built in the first place).

cc @nodejs/build

@refack
Copy link
Contributor

refack commented Oct 20, 2018

  1. 'cflags': [ '-Wall', '-Wextra', '-Wno-unused-parameter', '-std=c11' ]

    This could be added to

    {
    'target_name': 'napi_binding',
    'sources': [ 'napi_binding.c' ]
    },

  2. I think we'll need to move the test to /test/addons-napi, otherwise we'll need to add more singleton exception to te CI config... (Also that will eliminate singleton make targets)

addaleax pushed a commit that referenced this pull request Oct 20, 2018
Ref: #23585 (comment)

Python's `list.remove` will throw if the element is not found and also
it removes only the first occurrence.

This patch replaces the use of `list.remove` with a `filter` which
solves both of the above mentioned problems.

PR-URL: #23727
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
jasnell pushed a commit that referenced this pull request Oct 21, 2018
Ref: #23585 (comment)

Python's `list.remove` will throw if the element is not found and also
it removes only the first occurrence.

This patch replaces the use of `list.remove` with a `filter` which
solves both of the above mentioned problems.

PR-URL: #23727
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Trott
Copy link
Member

Trott commented Oct 22, 2018

Added a commit to skip the test on Debug builds.

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

@refack refack added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 23, 2018
@Trott
Copy link
Member

Trott commented Oct 24, 2018

CI is green! 🎉

@refack OK if we land this and you or @forivall or someone else can open a PR for the subsequent Makefile changes you suggest above?

@refack
Copy link
Contributor

refack commented Oct 24, 2018

OK if we land this and you or @forivall or someone else can open a PR for the subsequent Makefile changes you suggest above?

Yes 👍
And thank you for following up on my comment.

Trott pushed a commit to Trott/io.js that referenced this pull request Oct 24, 2018
Also makes sure that the napi benchmark is built before running jstest.

Skipped on windows since n-api benchmarks aren't built there yet.

PR-URL: nodejs#23585
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
@Trott
Copy link
Member

Trott commented Oct 24, 2018

Landed in 8c99a22.

Thanks for the contribution! 🎉

(If you're interested in other possible contributions to Node.js but don't have a good idea of where to start looking, some ideas are posted at https://www.nodetodo.org/next-steps/.)

@Trott Trott closed this Oct 24, 2018
targos pushed a commit that referenced this pull request Oct 26, 2018
Also makes sure that the napi benchmark is built before running jstest.

Skipped on windows since n-api benchmarks aren't built there yet.

PR-URL: #23585
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 30, 2018
Ref: #23585 (comment)

Python's `list.remove` will throw if the element is not found and also
it removes only the first occurrence.

This patch replaces the use of `list.remove` with a `filter` which
solves both of the above mentioned problems.

PR-URL: #23727
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 26, 2018
Also makes sure that the napi benchmark is built before running jstest.

Skipped on windows since n-api benchmarks aren't built there yet.

PR-URL: #23585
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
@codebytere codebytere mentioned this pull request Nov 27, 2018
rvagg pushed a commit that referenced this pull request Nov 28, 2018
Ref: #23585 (comment)

Python's `list.remove` will throw if the element is not found and also
it removes only the first occurrence.

This patch replaces the use of `list.remove` with a `filter` which
solves both of the above mentioned problems.

PR-URL: #23727
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
Also makes sure that the napi benchmark is built before running jstest.

Skipped on windows since n-api benchmarks aren't built there yet.

PR-URL: #23585
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
Ref: #23585 (comment)

Python's `list.remove` will throw if the element is not found and also
it removes only the first occurrence.

This patch replaces the use of `list.remove` with a `filter` which
solves both of the above mentioned problems.

PR-URL: #23727
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
Also makes sure that the napi benchmark is built before running jstest.

Skipped on windows since n-api benchmarks aren't built there yet.

PR-URL: #23585
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
@codebytere codebytere mentioned this pull request Nov 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events.
Projects
None yet
Development

Successfully merging this pull request may close these issues.