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

runtime: deprecate SetCPUProfileRate and replace body with panic #40094

Open
rsc opened this issue Jul 7, 2020 · 14 comments
Open

runtime: deprecate SetCPUProfileRate and replace body with panic #40094

rsc opened this issue Jul 7, 2020 · 14 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@rsc
Copy link
Contributor

rsc commented Jul 7, 2020

Long ago we had runtime.SetCPUProfileRate and runtime.CPUProfile as the interface to profiling. And runtime/pprof was layered on top of them. At some point we realized that this was not a good enough interface, and we made the runtime export hooks directly to runtime/pprof.

We marked CPUProfile deprecated and replaced the body with a panic.

But we forgot to do the same to SetCPUProfileRate. The result is that today this function is documented as if it is useful, when in fact it is not. We should mark it deprecated and replace the body with a panic, same as CPUProfile, leaving a back-door hook for runtime/pprof.

@rsc rsc added the NeedsFix The path to resolution is known, but the work has not been done. label Jul 7, 2020
@rsc rsc added this to the Go1.16 milestone Jul 7, 2020
@cherrymui
Copy link
Member

I think SetCPUProfileRate is, albeit confusing, still functioning. It needs to be called in the right order with StartCPUProfile. Last time I looked (~a month ago), it probably prints some warnings, but it does change the rate.

The pprof package doesn't seem to have a way to set the profiling rate. Not sure if this is really a useful feature.

@rhysh
Copy link
Contributor

rhysh commented Jul 26, 2020

The pprof package doesn't seem to have a way to set the profiling rate. Not sure if this is really a useful feature.

I have an unfortunate example of it being useful: Go's CPU profiling gives inaccurate results if the profiling rate is "too high". On the Linux machines I use, that's "more than 250 samples per second across the entire process". I've used SetCPUProfileRate to work around #35057, though only for the purpose of filing that bug report. It's the only practical workaround I know for that issue, and the warning it prints ("runtime: cannot set cpu profile rate until previous profile has finished." as of go1.13.3) is the main indication I see that it's not suitable for production use.

@rsc
Copy link
Contributor Author

rsc commented Sep 2, 2020

@cherrymui I don't see how SetCPUProfileRate can be useful.
It enables the profiler but there's no way to read the profiles back out.
If you later call pprof.StartCPUProfile, that errors out if the profiler is on,
and otherwise it calls SetCPUProfileRate(100).
What am I missing?

@cherrymui
Copy link
Member

If you later call pprof.StartCPUProfile, that errors out if the profiler is on,
and otherwise it calls SetCPUProfileRate(100).

Yeah. SetCPUProfileRate(100) then, seeing the profiler is already started, print a message and return, without changing the rate ( https://tip.golang.org/src/runtime/cpuprof.go#L65 ). It will collect profile at the rate set by the previous SetCPUProfileRate call.

@rhysh
Copy link
Contributor

rhysh commented Sep 2, 2020

If you later call pprof.StartCPUProfile, that errors out if the profiler is on

That checks runtime/pprof.cpu.profiling. It does not check runtime.cpuprof.on.

An early call to runtime.SetCPUProfileRate does not cause a subsequent call to pprof.StartCPUProfile to return an error (though it does cause it to print a log line). CPU profiling data at the rate requested by the first call will be written to the io.Writer provided in the second call.

@rsc
Copy link
Contributor Author

rsc commented Sep 3, 2020

OK, now I see how that could possibly work.
Do we actually want to support that?
It seems like an accident.

@cherrymui
Copy link
Member

The printed message suggests it is probably an accident (such that for a long time I thought it just doesn't work, until I found out the message is not actually an error).

Do we actually want to support that?

Personally, I only used it once, and I don't mind taking it out. It seems @rhysh used it only once as well. Not sure about other people's uses, though. (Maybe there are not many uses, as it appears not working.)

@tpaschalis
Copy link
Contributor

tpaschalis commented Sep 7, 2020

In my humble opinion, the way that profiling rates might work differently between OSes/kernel settings when you start nearing millisecond-scale resolutions is an issue as well.

I'd like to open a CL if there's no objection; I'll maintain the 100Hz hardcoded value and keep changes at a minimum.
Edit: I'm really sorry for opening a CL before consensus was reached in the discussion.

@gopherbot
Copy link

Change https://golang.org/cl/253398 mentions this issue: runtime: deprecate SetCPUProfileRate

@rogerlucena
Copy link

I have just opened a separate Issue (#42502) defending runtime.SetCPUProfileRate, or at least its functionality in the context of pprof.StartCPUProfile, explaining one use case on which it was important along with some supporting links as well.

@aclements
Copy link
Member

Given that this still needs a decision and there's a proposal for improvements working through the proposal process, I'm kicking this to 1.17. (Let me know if I misunderstood.)

@aclements aclements modified the milestones: Go1.16, Go1.17 Dec 15, 2020
@mknyszek
Copy link
Contributor

Since this issue is very much related to #42502 and although that issue did not make a decision on what to do here (AFAICT), that issue is in the backlog. So I'm going to move this to the backlog to align the two, under the assumption that nothing is going to happen here before the freeze (May 1st).

If that's incorrect, please let me know or feel free move it back. Just trying to clean out the milestone a bit.

@mknyszek mknyszek modified the milestones: Go1.17, Go1.18 Apr 15, 2021
@mknyszek
Copy link
Contributor

Moving this to backlog, though the discussion here seems related to @rhysh's recent work.

CC @prattmic

@BrennaEpp
Copy link

I'll add a comment here in case someone else gets bitten by this before (if) this gets deprecated:

Calling runtime.SetCPUProfileRate() also turns the profiling on, before the call to pprof.StartCPUProfile. I was getting extra functions that should not have been in the profile and it turns out runtime.SetCPUProfileRate() was the culprit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Projects
Status: Triage Backlog
Development

No branches or pull requests

9 participants