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

Conversation

@jasnell
Copy link
Member

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

jasnell added 2 commits Apr 26, 2017
v8: refactor struture of v8 module
Use the more efficient module.exports = {} pattern,
restructure imports from bindings, requires.
'getHeapSpaceStatistics'
],
n: [1e6],
flags: ['--ignition --turbo', '']

This comment has been minimized.

Copy link
@vsemozhetbyt

vsemozhetbyt Apr 27, 2017

Contributor

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.

This comment has been minimized.

Copy link
@vsemozhetbyt

vsemozhetbyt Apr 27, 2017

Contributor

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

This comment has been minimized.

Copy link
@jasnell

jasnell Apr 27, 2017

Author Member

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

This comment has been minimized.

Copy link
@vsemozhetbyt

vsemozhetbyt Apr 27, 2017

Contributor

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

This comment has been minimized.

Copy link
@vsemozhetbyt

vsemozhetbyt Apr 27, 2017

Contributor

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

This comment has been minimized.

Copy link
@vsemozhetbyt

vsemozhetbyt Apr 27, 2017

Contributor

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

This comment has been minimized.

Copy link
@jasnell

jasnell Apr 27, 2017

Author Member

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.

This comment has been minimized.

Copy link
@mscdex

mscdex Apr 27, 2017

Contributor

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

This comment has been minimized.

Copy link
@vsemozhetbyt

vsemozhetbyt Apr 27, 2017

Contributor

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.

This comment has been minimized.

Copy link
@joyeecheung

joyeecheung Apr 28, 2017

Member

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 label Apr 27, 2017

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Apr 27, 2017

@vsemozhetbyt ... updated! PTAL

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Apr 27, 2017

@joyeecheung

This comment has been minimized.

Copy link
Member

commented Apr 28, 2017

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
benchmark: add benchmark for v8.getHeap*Statistics
PR-URL: #12681
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
jasnell added a commit that referenced this pull request Apr 28, 2017
v8: refactor struture of v8 module
Use the more efficient module.exports = {} pattern,
restructure imports from bindings, requires.

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

This comment has been minimized.

Copy link
Member Author

commented Apr 28, 2017

Landed in bed4612 and 1052383

@jasnell jasnell closed this Apr 28, 2017

@evanlucas evanlucas referenced this pull request May 1, 2017
evanlucas added a commit that referenced this pull request May 1, 2017
benchmark: add benchmark for v8.getHeap*Statistics
PR-URL: #12681
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
evanlucas added a commit that referenced this pull request May 2, 2017
benchmark: add benchmark for v8.getHeap*Statistics
PR-URL: #12681
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
evanlucas added a commit that referenced this pull request May 2, 2017
benchmark: add benchmark for v8.getHeap*Statistics
PR-URL: #12681
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
evanlucas added a commit that referenced this pull request May 3, 2017
benchmark: add benchmark for v8.getHeap*Statistics
PR-URL: #12681
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
@jasnell jasnell referenced this pull request May 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.