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.

Sign up for GitHub

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

Jump to bottom

benchmark: add --expose_internals switch #8547

Closed
wants to merge 2 commits into from
Closed

benchmark: add --expose_internals switch #8547

wants to merge 2 commits into from
+6 −2

Conversation

bzoz
Copy link
Contributor

@bzoz 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

@bzoz bzoz added windows Issues and PRs related to the Windows platform. benchmark Issues and PRs related to the benchmark subsystem. labels Sep 15, 2016
@nodejs-github-bot nodejs-github-bot added the benchmark Issues and PRs related to the benchmark subsystem. label Sep 15, 2016
jasnell
jasnell approved these changes Sep 15, 2016
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM!

@gibfahn
Copy link
Member

gibfahn commented Sep 15, 2016

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

AndreasMadsen
AndreasMadsen reviewed Sep 15, 2016
View reviewed changes
benchmark/run.js
@@ -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'] });
Copy link
Member

@AndreasMadsen AndreasMadsen Sep 15, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

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

@AndreasMadsen AndreasMadsen added dont-land-on-v4.x benchmark Issues and PRs related to the benchmark subsystem. and removed benchmark Issues and PRs related to the benchmark subsystem. windows Issues and PRs related to the Windows platform. labels Sep 15, 2016
@AndreasMadsen
Copy link
Member

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).

Fishrock123
Fishrock123 reviewed Sep 15, 2016
View reviewed changes
benchmark/run.js
@@ -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'] });
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@AndreasMadsen AndreasMadsen Sep 16, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thats a great idea!

@bzoz
Copy link
Contributor Author

bzoz commented Sep 16, 2016

Updated, PTAL

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

@AndreasMadsen
Copy link
Member

AndreasMadsen commented Sep 16, 2016 •
edited
Loading

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
Copy link
Contributor Author

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.

@bzoz
benchmark: add --expose_internals switch
167b1e6 
Adds --expose_internals switch to benchmark runner. This makes
misc/freelist.js benchmark run properly
@bzoz bzoz force-pushed the bartek-fix-misc-freelist.js branch from 6cd4a0d to 167b1e6 Compare September 16, 2016 10:32
@bzoz
Copy link
Contributor Author

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
AndreasMadsen approved these changes Sep 16, 2016
View reviewed changes
Copy link
Member

@AndreasMadsen AndreasMadsen left a comment

Choose a reason for hiding this comment

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

LGTM

Fishrock123
Fishrock123 suggested changes Sep 17, 2016
View reviewed changes
benchmark/misc/freelist.js

var bench = common.createBenchmark(main, {
n: [100000]
});

function main(conf) {
const FreeList = require('internal/freelist').FreeList;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

jasnell
jasnell approved these changes Sep 21, 2016
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Still LGTM!

@bzoz
Copy link
Contributor Author

bzoz commented Sep 22, 2016

Updated, PTAL

@bzoz
Copy link
Contributor Author

bzoz commented Sep 22, 2016

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

@bzoz
Copy link
Contributor Author

bzoz commented Sep 26, 2016

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

bzoz added a commit that referenced this pull request Sep 26, 2016
@bzoz
benchmark: add --expose_internals switch
99a2dd0 
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
Copy link
Contributor Author

bzoz commented Sep 26, 2016

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

@bzoz bzoz closed this Sep 26, 2016
@AndreasMadsen
Copy link
Member

Awesome work 👍

jasnell pushed a commit that referenced this pull request Sep 29, 2016
@bzoz @jasnell
benchmark: add --expose_internals switch
84481f9 
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 pushed a commit to geek/node that referenced this pull request Sep 30, 2016
@bzoz @geek
benchmark: add --expose_internals switch
307199d 
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 mentioned this pull request Jun 15, 2017
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fishrock123 Fishrock123 Fishrock123 requested changes

@jasnell jasnell jasnell approved these changes

@AndreasMadsen AndreasMadsen AndreasMadsen approved these changes

Assignees
No one assigned
Labels
benchmark Issues and PRs related to the benchmark subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@bzoz @gibfahn @AndreasMadsen @jasnell @Fishrock123 @nodejs-github-bot