-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
v8: refactor the v8 module #12681
Conversation
Use the more efficient module.exports = {} pattern, restructure imports from bindings, requires.
benchmark/v8/get-stats.js
Outdated
'getHeapSpaceStatistics' | ||
], | ||
n: [1e6], | ||
flags: ['--ignition --turbo', ''] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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'
There was a problem hiding this comment.
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 :-)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@vsemozhetbyt ... updated! PTAL |
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 likeconst 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. |
PR-URL: #12681 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Use the more efficient module.exports = {} pattern, restructure imports from bindings, requires. PR-URL: #12681 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Landed in bed4612 and 1052383 |
PR-URL: #12681 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
PR-URL: #12681 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
PR-URL: #12681 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
PR-URL: #12681 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
module.exports = {}
pattern,Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
v8