profile.Start() should only be called once

[davecheney]

Now that we've made it impossible to activate more than one profile at a time, users may attempt to work around this with something like

 defer profile.Start(profile.CPUProfile).Stop
 defer profile.Start(profile.MemProfile).Stop

We should decide to support this, and if so, make sure that we move anything which is shared across profiles into the *Profile, see #5. Or, we should prohibit it with some package global lock.

[gbbr]

Judging solely by your previous remark about multiple profiles being explicitly prevented (#3), I would suggest having a package global lock (maybe a sync.Mutex as to not reinvent the wheel) and prohibiting this "hack".

Additionally, to ease flexibility, I was going to suggest earlier to possibly allow some special command line flags that would help the user re-run his application with various profiling settings without having the need to re-edit his source code to change the options. These flags would override any options passed to Start(). For example -profile.method={0,1,2}, -profile.path={path}, -profile.shutdownhook etc.

Keen to hear your thoughts.

[gbbr]

Upon further analysis a sync.Mutex would potentially cause a deadlock. So we would need a more custom solution. We could potentially ignore all future calls to Start and log the cancellation?

[davecheney]

On Wed, Oct 29, 2014 at 7:30 AM, Gabriel Aszalos notifications@github.com
wrote:

Judging solely by your previous remark about multiple profiles being explicitly
prevented (#3 #3), I would suggest
having a package global lock (maybe a sync.Mutex as to not reinvent the
wheel) and prohibiting this "hack".

A mutex sounds good, but probaby atomic.CompareAndSwap would be better.

// started counts the number of times the Start method has been called.
// It is used to prevent Start being called more than once.
var started uint32

func Start(...) ... {
if !atomic.CompareAndSwap(&started, 0, 1) {
log.Fatal("profile: Start() already called")
}

Additionally, to ease flexibility, I was going to suggest earlier to

possibly allow some special command line flags that would help the user
re-run his application with various profiling settings without having the
need to re-edit his source code to change the options. These flags would
override any options passed to Start(). For example
-profile.method={0,1,2}, -profile.path={path}, -profile.shutdownhook etc.

So this is one of the disadvantages of going from a struct to a option
function, we couldn't do

var cpuflag = flag.Bool("cpu", ... )

c := config { CPUProfile: *cpuflag }

I think we can handle this in the profile.defaultOptions method using some
flags.

Keen to hear your thoughts.

—
Reply to this email directly or view it on GitHub
#6 (comment).

[gbbr]

func Start(...) ... {
if !atomic.CompareAndSwap(&started, 0, 1) {
log.Fatal("profile: Start() already called")
}

I love this

So this is one of the disadvantages of going from a struct to a option
function, we couldn't do

I have an idea on how to do this nicely. Will update PR #5 later for you to look at.

[davecheney]

Yeah, sync.Mutex doesn't allow the caller to know if they failed to take
the lock. This is partially by design, java's Lock.tryLock is inherently
racy as you can fail to take the lock, but immediately after that the
holder releases it -- better to use a different primative like the atomic
CAS as a semaphore.

On Wed, Oct 29, 2014 at 7:38 AM, Gabriel Aszalos notifications@github.com
wrote:

Upon further analysis a sync.Mutex would potentially cause a deadlock. So
we would need a more custom solution. We could potentially ignore all
future calls to Start and log the cancellation?

—
Reply to this email directly or view it on GitHub
#6 (comment).

[gbbr]

I think for now we should settle for what we have discussed above, which is already in the PR.

But, I also think it's worth pondering further on how we can allow running multiple profiling modes because I'm pretty sure it would be common use case - unless this is specifically disallowed in stdlib. As far as I understood how they work, these profiles seem to always be created and it's just a matter of doing a Lookup (except the CPU one). If that's the case then I'd like to consider allowing simultaneous ones.

[gbbr]

This is now resolved. Flags will be separate if we decide too.