Skip to content
This repository has been archived by the owner on Jan 12, 2024. It is now read-only.

WIP: -p/--profile option starts v8/chromium/chrome compatible profiling. #485

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

WIP: -p/--profile option starts v8/chromium/chrome compatible profiling. #485

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8e2b0f2
Current status: -p/--profile option starts v8 compatible profiling.
StyXman Jul 28, 2017
f95bb92
Already imported.
StyXman Jul 28, 2017
b077c38
s/const/var/, because we are still using ES5.
StyXman Jul 28, 2017
1f3ca6a
Fix profiling.
StyXman Jul 29, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 20 additions & 5 deletions bin/carto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,16 +11,17 @@ var existsSync = require('fs').existsSync || require('path').existsSync

var yargs = require('yargs')
.usage("Usage: $0 <source MML file>")
.options('h', {alias:'help', describe:'Display this help message'})
.options('v', {alias:'version', boolean:true, describe:'Display version information'})
.options('a', {alias:'api', describe:'Specify Mapnik API version'})
.options('b', {alias:'benchmark', boolean:true, describe:'Outputs total compile time'})
.options('f', {alias:'file', describe:'Outputs to the given file instead of stdout.'})
.options('h', {alias:'help', describe:'Display this help message'})
.options('l', {alias:'localize', boolean:true, default:false, describe:'Use millstone to localize resources when loading an MML'})
.options('n', {alias:'nosymlink', boolean:true, describe:'Use absolute paths instead of symlinking files'})
.options('a', {alias:'api', describe:'Specify Mapnik API version'})
.options('f', {alias:'file', describe:'Outputs to the given file instead of stdout.'})
.options('o', {alias:'output', describe:'Specify output format (mapnik, json)', default:'mapnik'})
.options('p', {alias:'profile', boolean:true, default:false, describe:'Generate v8 profile data that can be loaded in chromium/chrome\'s profiler for inspection.'})
.options('q', {alias:'quiet', boolean:true, default:false, describe:'Do not output any warnings'})
.options('ppi', {describe:'Pixels per inch used to convert m, mm, cm, in, pt, pc to pixels', default:90.714});
.options('ppi', {describe:'Pixels per inch used to convert m, mm, cm, in, pt, pc to pixels', default:90.714})
Copy link
Collaborator

Choose a reason for hiding this comment

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

if you reorder options you should probably move that before q.

Copy link
Author

Choose a reason for hiding this comment

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

heeh, I missed that. in any case the reordering could be removed of the PR if you want.

.options('v', {alias:'version', boolean:true, describe:'Display version information'});

var options = yargs.argv;

Expand Down Expand Up @@ -67,6 +68,11 @@ try {
process.exit(1);
}

if (options.profile) {
const profiler = require('v8-profiler');
profiler.startProfiling('carto');
}

if (ext === '.mml') {
var mml = new carto.MML(options);
mml.load(path.dirname(input), data, compile);
Expand All @@ -76,6 +82,15 @@ if (ext === '.mml') {
console.error("carto: please pass either a .mml file or .mss file");
}

if (options.profile) {
const fs = require('fs');
Copy link
Collaborator

Choose a reason for hiding this comment

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

const is part of ES6, we are still using ES5 so it should be var.

Copy link
Author

Choose a reason for hiding this comment

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

ack.

const profile = profiler.stopProfiling('carto');

profile.export(function(error, result) {
fs.writeFileSync('carto.cpuprofile', result);
profile.delete();
})
}

function compile(err, data) {
if (err) {
Expand Down
8 changes: 7 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,17 @@
"yargs": "~8.0.1"
},
"devDependencies": {
"core-util-is": "^1.0.2",
"coveralls": "~2.13.1",
"inherits": "^2.0.3",
"istanbul": "~0.4.5",
"mocha": "~3.4.1",
"mocha-eslint": "^4.0.0",
"sax": "~1.2.1"
"npm": "^5.3.0",
"readable-stream": "^2.3.3",
"sax": "~1.2.1",
"util-deprecate": "^1.0.2",
"v8-profiler": "^5.7.0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wouldn't this dependency suffice? Why do we need all the others?

Copy link
Author

Choose a reason for hiding this comment

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

I had a problem while installing v8-profiler, it kept complaining about a missing dependency, so I just kept adding things to npm install until it worked. Also, notice that Travis and AppVeyor complain about v8-profiler, so probably we could leav it completely out, add a check in the code, and suggest the command to run.

},
"scripts": {
"pretest": "npm install && mocha -R spec --timeout 50000 -f jslint",
Expand Down