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

tools: change tick processor install path #4021

Closed
wants to merge 1 commit into from
Closed

tools: change tick processor install path #4021

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions .eslintignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
lib/internal/v8_prof_polyfill.js
lib/punycode.js
test/addons/doc-*/
test/fixtures
Expand Down
2 changes: 2 additions & 0 deletions doc/node.1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,8 @@ and servers.

--track-heap-objects track heap object allocations for heap snapshots

--prof-process process v8 profiler output generated using --prof

--v8-options print v8 command line options

--tls-cipher-list=list use an alternative default TLS cipher list
Expand Down
9 changes: 8 additions & 1 deletion tools/v8-prof/polyfill.js → lib/internal/v8_prof_polyfill.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,14 @@ arguments = process.argv.slice(2);
var quit = process.exit;

// Polyfill "readline()".
var fd = fs.openSync(arguments[arguments.length - 1], 'r');
var logFile = arguments[arguments.length - 1];
try {
fs.accessSync(logFile);
} catch(e) {
console.error('Please provide a valid isolate file as the final argument.');
process.exit(1);
}
var fd = fs.openSync(logFile, 'r');
var buf = new Buffer(4096);
var dec = new (require('string_decoder').StringDecoder)('utf-8');
var line = '';
Expand Down
44 changes: 44 additions & 0 deletions lib/internal/v8_prof_processor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
'use strict';
var cp = require('child_process');
var fs = require('fs');
var path = require('path');

var scriptFiles = [
'internal/v8_prof_polyfill',
'v8/tools/splaytree',
'v8/tools/codemap',
'v8/tools/csvparser',
'v8/tools/consarray',
'v8/tools/profile',
'v8/tools/profile_view',
'v8/tools/logreader',
'v8/tools/tickprocessor',
'v8/tools/SourceMap',
'v8/tools/tickprocessor-driver'
];
var tempScript = 'tick-processor-tmp-' + process.pid;
var tempNm = 'mac-nm-' + process.pid;

process.on('exit', function() {
try { fs.unlinkSync(tempScript); } catch (e) {}
try { fs.unlinkSync(tempNm); } catch (e) {}
});
process.on('uncaughtException', function(err) {
try { fs.unlinkSync(tempScript); } catch (e) {}
try { fs.unlinkSync(tempNm); } catch (e) {}
throw err;
});

scriptFiles.forEach(function(script) {
fs.appendFileSync(tempScript, process.binding('natives')[script]);
});
var tickArguments = [tempScript];
if (process.platform === 'darwin') {
fs.writeFileSync(tempNm, process.binding('natives')['v8/tools/mac-nm'],
{ mode: 0o555 });
tickArguments.push('--mac', '--nm=' + path.join(process.cwd(), tempNm));
} else if (process.platform === 'win32') {
tickArguments.push('--windows');
}
tickArguments.push.apply(tickArguments, process.argv.slice(1));
cp.spawn(process.execPath, tickArguments, { stdio: 'inherit' });
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to do this without writing temp files and forking a new process? I imagine you can just concatenate the files and then eval() or vm.runInNewContext() them.

(EDIT: It's perhaps a bit of a pain to send the output through c++filt. Shouldn't be impossible, though.)

Copy link
Author

Choose a reason for hiding this comment

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

This would definitely be a big improvement. The current state was the easiest way to get things working but I'm hoping to get rid of the temp files/forking in a future PR.

13 changes: 13 additions & 0 deletions node.gyp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,20 @@
'lib/internal/repl.js',
'lib/internal/socket_list.js',
'lib/internal/util.js',
'lib/internal/v8_prof_polyfill.js',
'lib/internal/v8_prof_processor.js',
'lib/internal/streams/lazy_transform.js',
'deps/v8/tools/splaytree.js',
'deps/v8/tools/codemap.js',
'deps/v8/tools/consarray.js',
'deps/v8/tools/csvparser.js',
'deps/v8/tools/profile.js',
'deps/v8/tools/profile_view.js',
'deps/v8/tools/logreader.js',
'deps/v8/tools/tickprocessor.js',
'deps/v8/tools/SourceMap.js',
'deps/v8/tools/tickprocessor-driver.js',
'deps/v8/tools/mac-nm',
],
},

Expand Down
14 changes: 13 additions & 1 deletion src/node.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ static const char** preload_modules = nullptr;
static bool use_debug_agent = false;
static bool debug_wait_connect = false;
static int debug_port = 5858;
static bool prof_process = false;
static bool v8_is_profiling = false;
static bool node_is_initialized = false;
static node_module* modpending;
Expand Down Expand Up @@ -2957,6 +2958,11 @@ void SetupProcessObject(Environment* env,
READONLY_PROPERTY(process, "throwDeprecation", True(env->isolate()));
}

// --prof-process
if (prof_process) {
READONLY_PROPERTY(process, "profProcess", True(env->isolate()));
}

// --trace-deprecation
if (trace_deprecation) {
READONLY_PROPERTY(process, "traceDeprecation", True(env->isolate()));
Expand Down Expand Up @@ -3194,6 +3200,8 @@ static void PrintHelp() {
" is detected after the first tick\n"
" --track-heap-objects track heap object allocations for heap "
"snapshots\n"
" --prof-process process v8 profiler output generated\n"
" using --prof\n"
" --v8-options print v8 command line options\n"
#if HAVE_OPENSSL
" --tls-cipher-list=val use an alternative default TLS cipher list\n"
Expand Down Expand Up @@ -3265,7 +3273,8 @@ static void ParseArgs(int* argc,
new_argv[0] = argv[0];

unsigned int index = 1;
while (index < nargs && argv[index][0] == '-') {
bool short_circuit = false;
Copy link
Member

Choose a reason for hiding this comment

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

Is this necessary?

Copy link
Author

Choose a reason for hiding this comment

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

This was the easiest way I could find to pass all args after --process-prof to the processing script. Is there a more idiomatic way to accomplish this?

Copy link
Member

Choose a reason for hiding this comment

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

I guess there isn't currently. It wasn't entirely clear to me it was for passing extra arguments verbatim.

while (index < nargs && argv[index][0] == '-' && !short_circuit) {
const char* const arg = argv[index];
unsigned int args_consumed = 1;

Expand Down Expand Up @@ -3326,6 +3335,9 @@ static void ParseArgs(int* argc,
track_heap_objects = true;
} else if (strcmp(arg, "--throw-deprecation") == 0) {
throw_deprecation = true;
} else if (strcmp(arg, "--prof-process") == 0) {
prof_process = true;
short_circuit = true;
} else if (strcmp(arg, "--v8-options") == 0) {
new_v8_argv[new_v8_argc] = "--help";
new_v8_argc += 1;
Expand Down
3 changes: 3 additions & 0 deletions src/node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,9 @@
var d = NativeModule.require('_debug_agent');
d.start();

} else if (process.profProcess) {
NativeModule.require('internal/v8_prof_processor');

} else {
// There is user code to be run

Expand Down
8 changes: 3 additions & 5 deletions test/parallel/test-tick-processor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@ var common = require('../common');

common.refreshTmpDir();
process.chdir(common.tmpDir);
var processor =
path.join(common.testDir, '..', 'tools', 'v8-prof', 'tick-processor.js');
// Unknown checked for to prevent flakiness, if pattern is not found,
// then a large number of unknown ticks should be present
runTest(/LazyCompile.*\[eval\]:1|.*% UNKNOWN/,
Expand Down Expand Up @@ -45,9 +43,9 @@ function runTest(pattern, code) {
assert.fail(null, null, 'There should be a single log file.');
}
var log = matches[0];
var out = cp.execSync(process.execPath + ' ' + processor +
' --call-graph-size=10 ' + log,
var out = cp.execSync(process.execPath +
' --prof-process --call-graph-size=10 ' + log,
{encoding: 'utf8'});
assert(out.match(pattern));
assert(pattern.test(out));
fs.unlinkSync(log);
}
44 changes: 0 additions & 44 deletions tools/install.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,48 +127,6 @@ def subdir_files(path, dest, action):
for subdir, files in ret.items():
action(files, subdir + '/')

def build_tick_processor(action):
tmp_script = 'out/Release/tick-processor'
if action == install:
# construct script
scripts = [
'tools/v8-prof/polyfill.js',
'deps/v8/tools/splaytree.js',
'deps/v8/tools/codemap.js',
'deps/v8/tools/csvparser.js',
'deps/v8/tools/consarray.js',
'deps/v8/tools/csvparser.js',
'deps/v8/tools/consarray.js',
'deps/v8/tools/profile.js',
'deps/v8/tools/profile_view.js',
'deps/v8/tools/logreader.js',
'deps/v8/tools/tickprocessor.js',
'deps/v8/tools/SourceMap.js',
'deps/v8/tools/tickprocessor-driver.js']
args = []
if sys.platform == 'win32':
args.append('--windows')
elif sys.platform == 'darwin':
args.append('--nm=' + abspath(install_path, 'share/doc/node') + '/mac-nm')
args.append('--mac')
with open(tmp_script, 'w') as out_file:
# Add #! line to run with node
out_file.write('#! ' + abspath(install_path, 'bin/node') + '\n')
# inject arguments
for arg in args:
out_file.write('process.argv.splice(2, 0, \'' + arg + '\');\n')
# cat in source files
for script in scripts:
with open(script) as in_file:
shutil.copyfileobj(in_file, out_file)
# make executable
st = os.stat(tmp_script)
os.chmod(tmp_script, st.st_mode | stat.S_IEXEC)
# perform installations
action([tmp_script], 'share/doc/node/')
if sys.platform == 'darwin':
action(['deps/v8/tools/mac-nm'], 'share/doc/node/')

def files(action):
is_windows = sys.platform == 'win32'

Expand All @@ -183,8 +141,6 @@ def files(action):

action(['deps/v8/tools/gdbinit'], 'share/doc/node/')

build_tick_processor(action)

if 'freebsd' in sys.platform or 'openbsd' in sys.platform:
action(['doc/node.1'], 'man/man1/')
else:
Expand Down
2 changes: 1 addition & 1 deletion tools/js2c.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,7 @@ def JS2C(source, target):
else:
ids.append((id, len(lines)))

escaped_id = id.replace('/', '_')
escaped_id = id.replace('-', '_').replace('/', '_')
source_lines.append(SOURCE_DECLARATION % {
'id': id,
'escaped_id': escaped_id,
Expand Down
4 changes: 0 additions & 4 deletions tools/rpm/node.spec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,17 +94,13 @@ done
/usr/include/*
/usr/lib/node_modules/
/usr/share/doc/node/gdbinit
/usr/share/doc/node/tick-processor
/usr/share/man/man1/node.1.gz
/usr/share/systemtap/tapset/node.stp
%{_datadir}/%{name}/
%doc CHANGELOG.md LICENSE README.md


%changelog
* Tue Sep 22 2015 Matt Loring <mattloring@google.com>
- Added tick processor.

* Tue Jul 7 2015 Ali Ijaz Sheikh <ofrobots@google.com>
- Added gdbinit.

Expand Down
51 changes: 0 additions & 51 deletions tools/v8-prof/tick-processor.js
View file
Open in desktop

This file was deleted.