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

Profile on start #2354

Closed
wants to merge 4 commits into from
Closed

Profile on start #2354

wants to merge 4 commits into from

Conversation

bmeck
Copy link
Member

@bmeck bmeck commented Aug 12, 2015

simple way to start the cpu profiler when you application starts up using --profile-cpu. useful for applications that need help profiling startup time.

@Qard
Copy link
Member

Qard commented Aug 12, 2015

Nice. 👍

@mscdex mscdex added the c++ Issues and PRs that require attention from people who are familiar with C++. label Aug 12, 2015
@r-52
Copy link
Contributor

r-52 commented Aug 12, 2015

I believe you could remove the static const char* - it's just an empty char, that the compiler would replace in the method call.
Another idea: let the user choose the name of the profiler title - the profiler title name could be the next value after your flag and if it isn't set, it becomes an empty string. Just to avoid potential collisions with addons that enable the profiler with the same profiler title name.

@@ -119,6 +119,8 @@ static bool throw_deprecation = false;
static bool abort_on_uncaught_exception = false;
static bool trace_sync_io = false;
static bool track_heap_objects = false;
static bool profile_cpu = false;
static const char* profile_title = "";
Copy link
Member

Choose a reason for hiding this comment

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

If the title is always empty, you might as well drop this variable and use String::Empty() below.

@bnoordhuis
Copy link
Member

Left some comments. I think the help section should make it clear that the flag is not related to --prof, otherwise it's going to be confusing to users.

@Fishrock123
Copy link
Contributor

@bmeck
Copy link
Member Author

bmeck commented Sep 8, 2015

@bnoordhuis did the slight rephrasing help? will need to rebase/squash before pulling in

@Fishrock123
Copy link
Contributor

@bnoordhuis LGTY?

@@ -3936,6 +3940,13 @@ static void StartNodeInstance(void* arg) {
Isolate::Scope isolate_scope(isolate);
HandleScope handle_scope(isolate);
Local<Context> context = Context::New(isolate);

// CpuProfiler requires HandleScope
// addons can pick up the results with StopProfiling(...) using same title
Copy link
Member

Choose a reason for hiding this comment

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

Please capitalize and punctuate the comment.

@bnoordhuis
Copy link
Member

@bmeck Can you rebase and squash to a single commit with a commit log conforming to the guidelines from CONTRIBUTING.md?

Anyone have thoughts on the name of the switch? I'm partial to --start-cpu-profiler.

@jasnell
Copy link
Member

jasnell commented Nov 16, 2015

@bmeck @bnoordhuis ... any further updates on this? @bmeck it would need to be rebased and updated before it could land.

@jasnell jasnell added the stalled Issues and PRs that are stalled. label Nov 16, 2015
@jasnell
Copy link
Member

jasnell commented Mar 22, 2016

Closing due to lack of activity or response

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. stalled Issues and PRs that are stalled.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants