Repository navigation
Cache program counters to frames - #62
Conversation
| // returned Frame only affects their own Stack and can never corrupt the | ||
| // shared cache (Frame is all strings and ints, so a struct copy is fully | ||
| // independent). | ||
| symCache sync.Map // map[uintptr][]Frame |
There was a problem hiding this comment.
Should we consider using something with a bounded size, or that expires entries? While the number of pc addresses might be statically determined, I'm not sure there's much value in keeping them around forever.
There was a problem hiding this comment.
Given we'll only populate the cache with frames from a Terror being constructed, I'm not overly worried about eviction?
Having constructed a Terror from that frame is probably a good signal that we'll eventually do it again in this process (or the process will go away, in which case freeing the memory doesn't matter).
There was a problem hiding this comment.
I talked about this here too: #62 (comment)
|
Oh, sorry, for some reason I thought you'd posted this in #tooling-club. |
EddShaw
left a comment
EddShaw
left a comment
There was a problem hiding this comment.
Have we measured the memory impact this might have on a typical service? I don't think I have a good idea of how many unique PCs we'd expect Terrors to see.
Looks good to me! I've left a few non-blocking comments, but I'm happy to ship this as is.
| name := fn.Name() | ||
| end := strings.LastIndex(name, string(os.PathSeparator)) | ||
| // Function names are always '/'-separated regardless of the host OS. | ||
| end := strings.LastIndex(name, "/") |
There was a problem hiding this comment.
Good thing we don't support Windows I guess. 🙈
| // A single PC can expand into multiple frames due to inlining, so we still | ||
| // have to iterate CallersFrames. Processing one PC at a time yields the same | ||
| // frames as processing the whole slice at once (verified in the tests). |
| return &b | ||
| }, | ||
| } | ||
|
|
There was a problem hiding this comment.
I've always known this operation as "symbolication", but I have seen "symbolisation" used elsewhere.
This probably doesn't matter, but I wonder if go uses one over the other? If so, it might be marginally easier to find relevant documentation if we're consistent. 👀
... Ok, it appears go uses neither, so never mind! 😅
| // returned Frame only affects their own Stack and can never corrupt the | ||
| // shared cache (Frame is all strings and ints, so a struct copy is fully | ||
| // independent). | ||
| symCache sync.Map // map[uintptr][]Frame |
There was a problem hiding this comment.
Given we'll only populate the cache with frames from a Terror being constructed, I'm not overly worried about eviction?
Having constructed a Terror from that frame is probably a good signal that we'll eventually do it again in this process (or the process will go away, in which case freeing the memory doesn't matter).
|
|
||
| // LoadOrStore keeps a single canonical slice per PC even if two goroutines | ||
| // race to symbolise the same one concurrently. | ||
| actual, _ := symCache.LoadOrStore(pc, frames) |
There was a problem hiding this comment.
Not blocking!
I'm a little anxious about turning this on for all terrors-using code all at once. It might be nice to feature-gate the cache? I wonder if we could hook up some feature-gating mechanism from bedrock?
There was a problem hiding this comment.
I guess the biggest concern is the cache sizing. I'm not concerned about this at all. We have a finite number of callsites which we can cache
The number of callsites we would symbolise in the pathological case is under 100,000, and even then we're looking at a couple megabytes of space usage
| // returned Frame only affects their own Stack and can never corrupt the | ||
| // shared cache (Frame is all strings and ints, so a struct copy is fully | ||
| // independent). | ||
| symCache sync.Map // map[uintptr][]Frame |
There was a problem hiding this comment.
We do have a typed sync map in wearedev in monzosync!
... Which we can't use because this is not in the monorepo. 😢
There was a problem hiding this comment.
We'd also need to worry about potential circular dependencies 🤔
This PR adds an in memory cache for symbolisation. This is helpful since for long running code, we have
terrorserrors defined in fixed locations and spend useless CPU time symbolising the same things over and overThere's a few other tweaks including
FuncForPCas we already pass the fully qualified function nameBenchmarks Before
Benchmarks After