Cache objectpath.Encoder.For() results to reduce CPU overhead#384
Cache objectpath.Encoder.For() results to reduce CPU overhead#384ewhauser wants to merge 2 commits intouber-go:mainfrom
Conversation
Add objPathCache map[types.Object]objectpath.Path to cache the results of objectpath.Encoder.For() calls. The same types.Object can be queried multiple times during analysis (shallow/deep checks, multiple triggers referencing the same site), and each For() call performs expensive recursive type traversal via objectpath.find(). Profiling shows objectpath.find() consuming 38-70% of CPU time in nogo builds. This cache eliminates redundant traversals for repeated queries of the same object.
|
|
Building on the caching from the previous commit, this adds fast paths to skip expensive objectpath.Encoder.For() calls: 1. Unexported non-types never have valid object paths - skip immediately 2. Package-level objects have simple paths (just the name) - compute directly This increases the performance an additional ~30%
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #384 +/- ##
==========================================
+ Coverage 87.01% 87.04% +0.02%
==========================================
Files 73 73
Lines 8305 8324 +19
==========================================
+ Hits 7227 7246 +19
Misses 885 885
Partials 193 193 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| // Fast path: unexported non-types never have a valid object path | ||
| _, isTypeName := obj.(*types.TypeName) | ||
| if !obj.Exported() && !isTypeName { | ||
| p.objPathCache[obj] = "" | ||
| return "" | ||
| } | ||
|
|
||
| // Fast path: package-level objects have a simple path (just the name) | ||
| if pkg := obj.Pkg(); pkg != nil { | ||
| if pkg.Scope().Lookup(obj.Name()) == obj { | ||
| path = objectpath.Path(obj.Name()) | ||
| p.objPathCache[obj] = path | ||
| return path | ||
| } | ||
| } |
There was a problem hiding this comment.
I'll admit that I don't remember the actual implementation for object path encoder, but these feel like these fast paths should just exist inside the encoder?
There was a problem hiding this comment.
@ewhauser @yuxincs Can we move this PR along? I can help. We effectively cannot use NilAway without this fix. See issue #411 for concrete OOM reproduction with before/after commits.
The question regarding encoder is reasonable, but practically it shouldn't block this PR.
- objectpath.Encoder is in golang.org/x/tools, a separate project with its own release cycle. Waiting for an upstream change there could take a long time.
- The cache is specific to nilaway's usage pattern (repeated calls for the same objects across triggers). Other users of objectpath.Encoder may not have this pattern, so upstream maintainers might not want the extra memory footprint.
| if err != nil { | ||
| path = "" | ||
| } | ||
| p.objPathCache[obj] = path |
There was a problem hiding this comment.
I'm not sure if a cache should just live inside the encoder too such that all callers get such performance boost (unless upstream does not really want to add the extra memory footprint?)
If so I'm not opposed to maintaining a cache here :) (I'll check if this increases a lot of memory consumption in our internal Go repo too)
|
This is nice! I left two comments more as discussion points rather than requests, let me know what you think! |
4 participants
Add objPathCache map[types.Object]objectpath.Path to cache the results of objectpath.Encoder.For() calls. The same types.Object can be queried multiple times during analysis (shallow/deep checks, multiple triggers referencing the same site), and each For() call performs expensive recursive type traversal via objectpath.find().
Profiling shows objectpath.find() consuming 40-70% of CPU time in our internal nogo builds. This cache eliminates redundant traversals for repeated queries of the same object and decreased the latency by roughly 7-8x for some packages.