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

benchmark: add --expose_internals switch #8547

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@bzoz
Contributor

bzoz commented Sep 15, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

benchmark

Description of change

Adds --expose_insternals switch to benchmark runner. This makes misc/freelist.js and any other benchmark requiring access to internal modules run properly on Windows.

cc @nodejs/benchmarking

@jasnell

LGTM!

@gibfahn

This comment has been minimized.

Member

gibfahn commented Sep 15, 2016

Nit: --expose_insternals -> --expose_internals in the commit message.

benchmark/run.js Outdated
@@ -36,7 +36,8 @@ if (format === 'csv') {
(function recursive(i) {
const filename = benchmarks[i];
const child = fork(path.resolve(__dirname, filename), cli.optional.set);
const child = fork(path.resolve(__dirname, filename), cli.optional.set,
{ execArgv: ['--expose_internals'] });

This comment has been minimized.

@AndreasMadsen

AndreasMadsen Sep 15, 2016

Member

This should extend process.execArgv not overwrite it. Otherwise profiling/debugging flags etc. won't work.

@AndreasMadsen

This comment has been minimized.

Member

AndreasMadsen commented Sep 15, 2016

I think you should add this to benchmark/common.js not benchmark/run.js. Otherwise you will have to add it too all files that forks the benchmarks (compare.js, scatter.js, run.js).

benchmark/run.js Outdated
@@ -36,7 +36,8 @@ if (format === 'csv') {
(function recursive(i) {
const filename = benchmarks[i];
const child = fork(path.resolve(__dirname, filename), cli.optional.set);
const child = fork(path.resolve(__dirname, filename), cli.optional.set,
{ execArgv: ['--expose_internals'] });

This comment has been minimized.

@Fishrock123

Fishrock123 Sep 15, 2016

Member

Doesn't this mean it will be passed regardless? DO we need the // Flags: comment since it doesn't work anyways?

This comment has been minimized.

@bzoz

bzoz Sep 16, 2016

Contributor

We don't need it, but it will be helpful for someone who tries to run this one benchmark

This comment has been minimized.

@AndreasMadsen

AndreasMadsen Sep 16, 2016

Member

common.js ensures that benchmarks are always forked, so there is no way to execute a single benchmark without --expose_internals being automatically applied.

This comment has been minimized.

@bzoz

bzoz Sep 16, 2016

Contributor

It is for people that try node benchmark\misc\freelist.js and encounter problems.

This comment has been minimized.

@AndreasMadsen

AndreasMadsen Sep 16, 2016

Member

Don't you think it would be better to move the require('internal/freelist') inside main, instead of having all these --expose_internals around?

This comment has been minimized.

@bzoz

bzoz Sep 16, 2016

Contributor

Thats a great idea!

@bzoz

This comment has been minimized.

Contributor

bzoz commented Sep 16, 2016

Updated, PTAL

Unfortunately, we need to add this everywhere - in compare.js, scatter.js and run.js.

@AndreasMadsen

This comment has been minimized.

Member

AndreasMadsen commented Sep 16, 2016

Unfortunately, we need to add this everywhere - in compare.js, scatter.js and run.js.

Please elaborate on why you can't just add it here: https://github.com/nodejs/node/blob/master/benchmark/common.js#L139L141

@bzoz

This comment has been minimized.

Contributor

bzoz commented Sep 16, 2016

When run.js runs benchmarker file it will fail on require('internals/..') before reaching common.createBenchmark. It will never reach that line in common.js.

benchmark: add --expose_internals switch
Adds --expose_internals switch to benchmark runner. This makes
misc/freelist.js benchmark run properly

@bzoz bzoz force-pushed the janeasystems:bartek-fix-misc-freelist.js branch to 167b1e6 Sep 16, 2016

@bzoz

This comment has been minimized.

Contributor

bzoz commented Sep 16, 2016

Updated again. @AndreasMadsen you had great idea. Moved the require to main, now only common.js needs the --expose_internals and everything works.

@AndreasMadsen

LGTM

var bench = common.createBenchmark(main, {
n: [100000]
});
function main(conf) {
const FreeList = require('internal/freelist').FreeList;

This comment has been minimized.

@Fishrock123

Fishrock123 Sep 17, 2016

Member

What's up with this? Maybe add a comment?

This comment has been minimized.

@bzoz

bzoz Sep 21, 2016

Contributor

require was moved there, so it will work with adding -expose_internals only in common.js. It was the result of discusion here: #8547 (review)

@Trott Trott force-pushed the nodejs:master branch to c5ce7f4 Sep 21, 2016

@jasnell

Still LGTM!

@bzoz

This comment has been minimized.

Contributor

bzoz commented Sep 22, 2016

Updated, PTAL

@bzoz

This comment has been minimized.

Contributor

bzoz commented Sep 22, 2016

If there are no further objections I would like to land this tomorrow.

@bzoz

This comment has been minimized.

Contributor

bzoz commented Sep 26, 2016

bzoz added a commit that referenced this pull request Sep 26, 2016

benchmark: add --expose_internals switch
Adds --expose_internals switch to benchmark runner. This makes
misc/freelist.js benchmark run properly

Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
PR-URL: #8547
@bzoz

This comment has been minimized.

Contributor

bzoz commented Sep 26, 2016

Landed in 99a2dd0. @AndreasMadsen - again, thanks for great input.

@bzoz bzoz closed this Sep 26, 2016

@AndreasMadsen

This comment has been minimized.

Member

AndreasMadsen commented Sep 26, 2016

Awesome work 👍

jasnell added a commit that referenced this pull request Sep 29, 2016

benchmark: add --expose_internals switch
Adds --expose_internals switch to benchmark runner. This makes
misc/freelist.js benchmark run properly

Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
PR-URL: #8547

geek added a commit to geek/node that referenced this pull request Sep 30, 2016

benchmark: add --expose_internals switch
Adds --expose_internals switch to benchmark runner. This makes
misc/freelist.js benchmark run properly

Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
PR-URL: nodejs#8547

@gibfahn gibfahn referenced this pull request Jun 15, 2017

Closed

Auditing for 6.11.1 #230

2 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment