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

v8: refactor the v8 module #12681

Closed
wants to merge 3 commits into from
Closed

v8: refactor the v8 module #12681

wants to merge 3 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Apr 26, 2017

  • Use the more efficient module.exports = {} pattern,
  • Refactor the imports from bindings and requires
  • Add a benchmark
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

v8

Use the more efficient module.exports = {} pattern,
restructure imports from bindings, requires.
'getHeapSpaceStatistics'
],
n: [1e6],
flags: ['--ignition --turbo', '']
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Apr 27, 2017

Choose a reason for hiding this comment

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

If I understand the guide right, this should be a property of the third argument (options). And flags do not vary, so empty flag seems to be useless. However, I may miss something.

Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Apr 27, 2017

Choose a reason for hiding this comment

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

Also I am not sure if the '--ignition --turbo' should be '--ignition', '--turbo'

Copy link
Member Author

Choose a reason for hiding this comment

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

Setting the flags like this runs the benchmark with two separate configurations, one with --ignition --turbo and the other without flags. I wasn't sure myself but this definitely worked when I ran it like this :-)

Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Apr 27, 2017

Choose a reason for hiding this comment

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

Are you sure the flags are really set, not only displayed in the config list? I've added a debug log:

'use strict';

const common = require('../common.js');
const v8 = require('v8');

const bench = common.createBenchmark(main, {
  method: [
    'getHeapStatistics',
    'getHeapSpaceStatistics'
  ],
  n: [1e6],
  flags: ['--ignition --turbo', '']
});

function main(conf) {
  console.log(process.execArgv);
  const n = +conf.n;
  const method = conf.method;
  var i = 0;
  bench.start();
  for (; i < n; i++)
    v8[method]();
  bench.end(n);
}
[]
v8\get-stats.js flags="--ignition --turbo" n=1000000 method="getHeapStatistics": 1,476,245.8999447592
[]
v8\get-stats.js flags="" n=1000000 method="getHeapStatistics": 1,455,785.4873297967
[]
v8\get-stats.js flags="--ignition --turbo" n=1000000 method="getHeapSpaceStatistics": 2,515,738.0104498775
[]
v8\get-stats.js flags="" n=1000000 method="getHeapSpaceStatistics": 2,505,423.9547382533

Copy link
Contributor

Choose a reason for hiding this comment

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

Compare:

'use strict';

const common = require('../common.js');
const v8 = require('v8');

const bench = common.createBenchmark(main, {
  method: [
    'getHeapStatistics',
    'getHeapSpaceStatistics'
  ],
  n: [1e6],
}, { flags: ['--ignition', '--turbo'] });

function main(conf) {
  console.log(process.execArgv);
  const n = +conf.n;
  const method = conf.method;
  var i = 0;
  bench.start();
  for (; i < n; i++)
    v8[method]();
  bench.end(n);
}
[ '--ignition', '--turbo' ]
v8\get-stats.js n=1000000 method="getHeapStatistics": 1,267,677.6893168623
[ '--ignition', '--turbo' ]
v8\get-stats.js n=1000000 method="getHeapSpaceStatistics": 1,821,113.6853929006

Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Apr 27, 2017

Choose a reason for hiding this comment

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

It seems this is a design that can be improved. Currently, the key flags in configs has no automatic impact, while the same flag in options can not vary between runs.

cc @nodejs/performance, @nodejs/benchmarking, @mscdex

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll just back the flags bit out of this particular change to keep it from holding this up at all, but big +1 on refactoring this generally.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would actually prefer flags in the config object to not have any automagic behavior. It should retain its 'config' meaning (allowing it to be specified on the command line as a regular benchmark option).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I did not propose the automatic behavior for this key in config. I meant that may be an easier way to vary execArgv between runs can be supported.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, the flags in the options is applied to all runs...to compare the performance under different flags I would suggest something like

const bench = common.createBenchmark(main, {
  method: [
    'getHeapStatistics',
    'getHeapSpaceStatistics'
  ],
  n: [1e6],
  pipeline: ['new', 'old']
});

function main(conf) {
  if (conf.pipeline === 'new') {
    v8.setFlagsFromString('--ignition');
    v8.setFlagsFromString('--turbo')
  }
}

Unless we have to turn on the flag during bootstrapping..in that case we do need to add special support in the benchmark suite.

@mscdex mscdex added the v8 engine Issues and PRs related to the V8 dependency. label Apr 27, 2017
@jasnell
Copy link
Member Author

jasnell commented Apr 27, 2017

@vsemozhetbyt ... updated! PTAL

@jasnell
Copy link
Member Author

jasnell commented Apr 27, 2017

@joyeecheung
Copy link
Member

Replied here since the review above is folded..

Yes, the `flags` in the `options` is applied to all runs...to compare the performance under different flags I would suggest something like
const bench = common.createBenchmark(main, {
  method: [
    'getHeapStatistics',
    'getHeapSpaceStatistics'
  ],
  n: [1e6],
  pipeline: ['new', 'old']
});

function main(conf) {
  if (conf.pipeline === 'new') {
    v8.setFlagsFromString('--ignition');
    v8.setFlagsFromString('--turbo')
  }
}

Unless we have to turn on the flag during bootstrapping..in that case we do need to add special support in the benchmark suite.

jasnell added a commit that referenced this pull request Apr 28, 2017
PR-URL: #12681
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
jasnell added a commit that referenced this pull request Apr 28, 2017
Use the more efficient module.exports = {} pattern,
restructure imports from bindings, requires.

PR-URL: #12681
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
@jasnell
Copy link
Member Author

jasnell commented Apr 28, 2017

Landed in bed4612 and 1052383

@jasnell jasnell closed this Apr 28, 2017
@evanlucas evanlucas mentioned this pull request May 1, 2017
evanlucas pushed a commit that referenced this pull request May 1, 2017
PR-URL: #12681
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
evanlucas pushed a commit that referenced this pull request May 2, 2017
PR-URL: #12681
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
evanlucas pushed a commit that referenced this pull request May 2, 2017
PR-URL: #12681
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
evanlucas pushed a commit that referenced this pull request May 3, 2017
PR-URL: #12681
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
@jasnell jasnell mentioned this pull request May 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants