Report GCP profiles from zoekt-git-index #816
Conversation
jtibshirani
force-pushed
the
jtibs/profile
branch
from
36de188
to
7feadfd
Compare
jtibshirani
changed the title
jtibshirani
changed the title
jtibshirani
force-pushed
the
jtibs/profile
branch
from
7feadfd
to
8ab6a84
Compare
|
Looking for feedback on this one :) It's a bit weird to be initializing the GCP profiler in a subprocess, especially since GCP collects data every 1 minute for 10 seconds. However, the data looks quite good from local testing: 
And in many of the cases we want to improve, indexing can take several minutes, and we can probably learn from this data. So it feels fine to have this as long as we understand the slow sampling rate compared to the process duration. What do you think? |
cmd/zoekt-git-index/main.go
Outdated
| @@ -107,6 +110,7 @@ func run() int { | |||
| opts.LanguageMap[m[0]] = ctags.StringToParser(m[1]) | |||
| } | |||
|
|
|||
| profiler.InitLightweight("zoekt-git-index", zoekt.Version) | |||
There was a problem hiding this comment.
What's the reasoning for having a separate profiler config? Is mutex profiling polluting zoekt-git-index and that's not the case for webserver or indexserver?
There was a problem hiding this comment.
Agreed, interesting in why we need a lighterweight indexing config here. Given this will only trigger for slower runs of zoekt-git-index, more info seems useful?
There was a problem hiding this comment.
The thing I found concerning was AllocForceGC: true. If I'm understanding correctly, it means that enabling profiling could significantly impact memory usage (lower peak usage due to more frequent GCs). So you could have weird things like "we switched off the GCP profiler and now memory usage is much higher."
However, it looks like we do this all over the place, including sg/sg, and I haven't heard of a problem. I'm probably just scarred from my Java days where WAY more stuff is on the heap and GC behavior is very core to memory usage.
I'll remove InitLightweight!
There was a problem hiding this comment.
Oh man I thought up so much crazy overengineering to accomplish this. This is so nice and simple, awesome!
And in many of the cases we want to improve, indexing can take several minutes, and we can probably learn from this data.
yeah in fact that is perfect to only have our slow index jobs. I suspect the vast majority of zoekt-git-index calls finish in seconds, so only seeing the slow ones is great for having a better understanding of what we need to improve.
internal/profiler/profiler.go
Outdated
| @@ -21,3 +21,17 @@ func Init(svcName, version string, blockProfileRate int) { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| // InitLightweight starts the supported profilers IFF the environment variable is set. | |||
| // Compared to Init, it disables mutex profiling and forced GC to reduce its overhead. | |||
There was a problem hiding this comment.
I actually don't know why we do AllocForceGC given the default is off. I wonder if it has a measurable impact on our perf. I suspect not. Then it makes sense. Why don't we use it for zoekt-git-index then?
cmd/zoekt-git-index/main.go
Outdated
| @@ -107,6 +110,7 @@ func run() int { | |||
| opts.LanguageMap[m[0]] = ctags.StringToParser(m[1]) | |||
| } | |||
|
|
|||
| profiler.InitLightweight("zoekt-git-index", zoekt.Version) | |||
There was a problem hiding this comment.
Agreed, interesting in why we need a lighterweight indexing config here. Given this will only trigger for slower runs of zoekt-git-index, more info seems useful?
This PR initializes the GCP profiler in the
zoekt-git-indexprocess so we canexamine CPU and memory usage for the indexing process itself.