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 · 11 comments
Open

runtime: deprecate SetCPUProfileRate and replace body with panic #40094

rsc opened this issue Jul 7, 2020 · 11 comments
Labels
Milestone

Comments

@rsc
Copy link
Contributor

@rsc 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 label Jul 7, 2020
@rsc rsc added this to the Go1.16 milestone Jul 7, 2020
@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Jul 7, 2020

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 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 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
Contributor

@cherrymui cherrymui commented Sep 2, 2020

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 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 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
Contributor

@cherrymui cherrymui commented Sep 3, 2020

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 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

@gopherbot gopherbot commented Sep 7, 2020

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

@rogerlucena
Copy link

@rogerlucena rogerlucena commented Nov 11, 2020

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

@aclements aclements commented Dec 15, 2020

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants