profiler: ensure that CPU profile records profiler work #1485
Conversation
Our profiler library does post-processing of profiles, such as computing deltas for heap, block, and mutex profiles, which has non-zero CPU overhead. Right now there's no guarantee that that work will be captured in the CPU profile. Add synchronization so that the CPU profiler will only be stopped after all other profiles are completed.
| @@ -159,8 +168,9 @@ var profileTypes = map[ProfileType]profileType{ | |||
| }, | |||
| } | |||
|
|
|||
| func collectGenericProfile(name string, delta *pprofutils.Delta) func(p *profiler) ([]byte, error) { | |||
| func collectGenericProfile(name string, pt ProfileType, delta *pprofutils.Delta) func(p *profiler) ([]byte, error) { | |||
There was a problem hiding this comment.
So, I tried just making collectGenericProfile just take a ProfileType argument and drop the less type-safe name argument. However, this creates a initialization cycle between the profileTypes map and the collectGenericProfile function, since we still want to get the profile name for doing the lookup, and getting the name requires accessing profileTypes.
Not sure how much refactoring to do in this PR. I could give ProfileType a name() string method that just does a switch statement, and remove Name from profileType? Or leave it kind of awkward for now and refactor in a subsequent PR?
There was a problem hiding this comment.
Leave it awkward for now, it's ok. We'll come back to it.
|
As-is, this PR breaks EDIT: changed the CPU profiler to wait until the end of the profiling period. |
If the CPU profile duration is less than the profiling period, make sure that the CPU profile happens at the end rather than the beginning of the profile period. That way, we respect the shorter CPU duration but still capture the profiler library work.
There was a problem hiding this comment.
LGTM, but I think a wait group would simplify the implementation quite a bit.
How have you tested this? I know it's difficult to write automated tests for this, but IIRC we've had some regression on this concurrency stuff in the past (7dfa5e8, 85c05b2). So at least we should do some manual and/or rel env testing to confirm that this works as intended and captures the delta profiling CPU work.
Simpler than a map of channels
I tested this manually. I ran a test app I've been using for other things. I had to increase the CPU profile frequency to pick up delta profiling since it was pretty fast for this app, but it shows up in the CPU profile with this PR applied: |
| @@ -159,8 +168,9 @@ var profileTypes = map[ProfileType]profileType{ | |||
| }, | |||
| } | |||
|
|
|||
| func collectGenericProfile(name string, delta *pprofutils.Delta) func(p *profiler) ([]byte, error) { | |||
| func collectGenericProfile(name string, pt ProfileType, delta *pprofutils.Delta) func(p *profiler) ([]byte, error) { | |||
There was a problem hiding this comment.
Leave it awkward for now, it's ok. We'll come back to it.
Accidentally removed by #1485
* profiler: add the Name field back for the CPU profile Accidentally removed by #1485. Also make the log output test more specific, which would have caught this.
Our profiler library does post-processing of profiles, such as computing
deltas for heap, block, and mutex profiles, which has non-zero CPU
overhead. Right now there's no guarantee that that work will be captured
in the CPU profile. Add synchronization so that the CPU profiler will
only be stopped after all other profiles are completed.