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 https://github.com/pkg/profile/pull/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 https://github.com/pkg/profile/issues/6#issuecomment-60825949.

[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 https://github.com/pkg/profile/issues/6#issuecomment-60827200.

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