Skip to content

Added command-line option '/amt' and '/ambt' to profile an existing process's certain thread by setting its process id#47

Closed
RammerChoi wants to merge 2 commits intoVerySleepy:masterfrom
RammerChoi:master
Closed

Added command-line option '/amt' and '/ambt' to profile an existing process's certain thread by setting its process id#47
RammerChoi wants to merge 2 commits intoVerySleepy:masterfrom
RammerChoi:master

Conversation

@RammerChoi
Copy link
Copy Markdown
Contributor

'/amt' is an option to profile "main thread".
And '/ambt' is an option to profile "most busy thread".

These were added because I needed.
But it may helpful to someone else, so I open a pull request.
please consider this request.

@CyberShadow
Copy link
Copy Markdown
Member

Could you please split out the refactoring in the second commit into a commit of its own?

catch (const std::exception&)
{
throw SleepyException("Not valid process id: " + processId);
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please refactor the common code in AttachToProcess, AttachToMostBusyThread, and AttachToMostBusyThread to avoid repetition.

Perhaps an attachMode enum parameter?

{ wxCMD_LINE_OPTION, "r", "", "Runs an executable and profiles it.", wxCMD_LINE_VAL_STRING, wxCMD_LINE_PARAM_OPTIONAL|wxCMD_LINE_NEEDS_SEPARATOR },
{ wxCMD_LINE_OPTION, "a", "", "Attaches to a process (by its PID) and profiles it.", wxCMD_LINE_VAL_STRING, wxCMD_LINE_PARAM_OPTIONAL|wxCMD_LINE_NEEDS_SEPARATOR },
{ wxCMD_LINE_OPTION, "amt", "", "Attaches to a process's main thread (by its PID) and profiles it.", wxCMD_LINE_VAL_STRING, wxCMD_LINE_PARAM_OPTIONAL | wxCMD_LINE_NEEDS_SEPARATOR },
{ wxCMD_LINE_OPTION, "ambt", "", "Attaches to a process's most busy thread (by its PID) and profiles it.", wxCMD_LINE_VAL_STRING, wxCMD_LINE_PARAM_OPTIONAL | wxCMD_LINE_NEEDS_SEPARATOR },
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it would make more sense to have a separate parameter to indicate which threads to attach (i.e. /a /mt instead of /amt).

@RammerChoi
Copy link
Copy Markdown
Contributor Author

Thank you for your advice.
I will fix that tonight.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants