-
Notifications
You must be signed in to change notification settings - Fork 250
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
Add cpu profiling #2536
Add cpu profiling #2536
Changes from 3 commits
de5a46f
a4f2bbf
76944f7
6a3825a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,6 +27,7 @@ const BROKEN_PIPE_ERROR_CODE = 'EPIPE'; | |
const IPC_CHANNEL_CLOSED_ERROR_CODE = 'ERR_IPC_CHANNEL_CLOSED'; | ||
const TIMEOUT_FOR_DISPOSE = 2000; | ||
|
||
let n = 0; | ||
export default class ChildProcessProxy<T> implements Disposable { | ||
public readonly proxy: Promisified<T>; | ||
|
||
|
@@ -48,7 +49,14 @@ export default class ChildProcessProxy<T> implements Disposable { | |
additionalInjectableValues: unknown, | ||
workingDirectory: string | ||
) { | ||
this.worker = fork(require.resolve('./ChildProcessProxyWorker'), [autoStart], { silent: true, execArgv: [] }); | ||
const execArgv = []; | ||
if (process.env.STRYKER_PROFILE) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think adding environment variables should be a "last resort". (I literally have nightmares about the vue-cli and the zoo of env variables there 😪). Can we simply check if the current process has the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, we can, I initially had it set up with a flag. The downside here is that we will have to add the flag to the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See commit, I've changed things over to the |
||
execArgv.push('--cpu-prof'); | ||
execArgv.push(`--cpu-prof-name=child-process-${n}.cpuprofile`); | ||
n++; | ||
} | ||
|
||
this.worker = fork(require.resolve('./ChildProcessProxyWorker'), [autoStart], { silent: true, execArgv: execArgv }); | ||
this.initTask = new Task(); | ||
this.log.debug('Starting %s in child process %s', requirePath, this.worker.pid); | ||
this.send({ | ||
|
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.
What is this doing here?
n
is never updated.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.
n
is updated on 56 and used on 55. Basically, I use it to enumerate the child process output files to check how many of them are missing. Another naming scheme is fine with me.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.
Should we not reuse this
n
? For example, with--concurrency 4
, we might want to use1
,2
,3
, and4
, but never5
. Whenever a child process crashes, it can reuse the number.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.
No, that'll result in overwritten
.cpuprofile
files.n
here is not a good solution by any means, but it is a human-readable approach to unique file names. Doing it sequenced like this allows a human to see how many processes crashed, as crashed processes have no.cpuprofile
file.For example:
We can see that 1, 3, 6-10, 12-14, and 17 are missing here. These processes must have crashed for some reason.
This insight is the only reason why we need
n
.Edit: The last file of the run is
child-process-278.cpuprofile
. I have a feeling that there are many more processes than you're expecting.