btcd: Add memory profiling flag #1953
Conversation
Pull Request Test Coverage Report for Build 4475766792
💛 - Coveralls |
| return err | ||
| } | ||
| defer f.Close() | ||
| defer pprof.WriteHeapProfile(f) |
There was a problem hiding this comment.
since the file is only ever written to on shutdown, can't this whole if statement be put in a defer? that way we also dont need to keep the file open for the lifetime of the binary
There was a problem hiding this comment.
I think it'd be nice if it errors out immediately if it's not able to create a file to write to.
I'm not sure on the costs of having a file descriptor for the duration of the program but if that's a concern, how about something like this:
// Write mem profile if requested.
if cfg.MemoryProfile != "" {
f, err := os.Create(cfg.MemoryProfile)
if err != nil {
btcdLog.Errorf("Unable to create memory profile: %v", err)
return err
}
f.Close()
defer func() {
f, err := os.Create(cfg.MemoryProfile)
if err != nil {
btcdLog.Infof("Unable to create memory profile: %v", err)
}
err = pprof.WriteHeapProfile(f)
if err != nil {
btcdLog.Infof("Unable to create memory profile: %v", err)
}
f.Close()
}()
}
There was a problem hiding this comment.
good point regarding failing fast on start up 👍 in that case, if the cost of the file descriptor is low then your original solution seems good enough!
There was a problem hiding this comment.
I think the cost is pretty low
There was a problem hiding this comment.
Ok I'll keep the current implementation.
|
cc @Crypt-iQ |
| return err | ||
| } | ||
| defer f.Close() | ||
| defer pprof.WriteHeapProfile(f) |
There was a problem hiding this comment.
I think the cost is pretty low
| } | ||
| defer f.Close() | ||
| defer pprof.WriteHeapProfile(f) | ||
| } |
There was a problem hiding this comment.
could add a defer runtime.GC() call at the end so that the heap profile is accurate as far as what's actually lingering
There was a problem hiding this comment.
@Roasbeef any opinions on this? It'll give us better data but it may slow things down during shutdown for normal users.
There was a problem hiding this comment.
IIUC, that would only be active for users already running with a flag that strictly reserved for profiling. So I think that's fine.
There was a problem hiding this comment.
Added a runtime.GC() so that garbage collection happens before writes
Enables Go memory profiling. If the cpuprofile shows a lot of time spent on gc, it's useful to then do a memory profile to see where the memory alloctions happen. Unlike the --profile flag, this allows for easy generation of a memory profile for the entire duration of which btcd has been running for in various readble graphs.
kcalvinalvin
force-pushed
the
add-memory-profiling
branch
from
8478416 to
d628705
Compare
Enables Go memory profiling. If the cpuprofile shows a lot of time spent on gc, it's useful to then do a memory profile to see where the memory alloctions happen.
Unlike the --profile flag, this allows for easy generation of a memory profile for the entire duration of which btcd has been running for in various readble graphs.