Skip to content

Cache objectpath.Encoder.For() results to reduce CPU overhead #384

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ewhauser wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Cache objectpath.Encoder.For() results to reduce CPU overhead #384

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3296e18
Cache objectpath.Encoder.For() results to reduce CPU overhead
ewhauser Jan 3, 2026
d6c92c3
Add fast paths to objectPath to reduce CPU overhead
ewhauser Jan 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 43 additions & 6 deletions inference/primitive.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package inference
import (
"fmt"
"go/token"
"go/types"

"go.uber.org/nilaway/annotation"
"go.uber.org/nilaway/util/analysishelper"
Expand Down Expand Up @@ -124,6 +125,9 @@ type primitivizer struct {
// objPathEncoder is used to encode object paths, which amortizes the cost of encoding the
// paths of multiple objects.
objPathEncoder *objectpath.Encoder
// objPathCache caches the results of objectPath() to avoid redundant expensive lookups
// for the same types.Object.
objPathCache map[types.Object]objectpath.Path
}

// newPrimitivizer returns a new and properly-initialized primitivizer.
Expand Down Expand Up @@ -170,6 +174,7 @@ func newPrimitivizer(pass *analysishelper.EnhancedPass) *primitivizer {
pass: pass,
upstreamObjPositions: upstreamObjPositions,
objPathEncoder: &objectpath.Encoder{},
objPathCache: make(map[types.Object]objectpath.Path),
}
}

Expand All @@ -191,12 +196,8 @@ func (p *primitivizer) fullTrigger(trigger annotation.FullTrigger) primitiveFull

// site returns the primitive version of the annotation site.
func (p *primitivizer) site(key annotation.Key, isDeep bool) primitiveSite {
objPath, err := p.objPathEncoder.For(key.Object())
if err != nil {
// An error will occur when trying to get object path for unexported objects, in which case
// we simply assign an empty object path.
objPath = ""
}
obj := key.Object()
objPath := p.objectPath(obj)

pkgRepr := ""
if pkg := key.Object().Pkg(); pkg != nil {
Expand Down Expand Up @@ -230,6 +231,42 @@ func (p *primitivizer) site(key annotation.Key, isDeep bool) primitiveSite {
}
}

// objectPath returns the objectpath.Path for the given object, using fast paths where possible
// to avoid the expensive traversal in objectpath.Encoder.For().
func (p *primitivizer) objectPath(obj types.Object) objectpath.Path {
// Check cache first
if path, ok := p.objPathCache[obj]; ok {
return path
}

var path objectpath.Path

// 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
}
}
Comment on lines +244 to +258
Copy link
Contributor

@yuxincs yuxincs Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

@getvictor getvictor Mar 20, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. 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.
  2. 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.


// Slow path: use the full objectpath encoder
var err error
path, err = p.objPathEncoder.For(obj)
if err != nil {
path = ""
}
p.objPathCache[obj] = path
Copy link
Contributor

@yuxincs yuxincs Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

return path
}

// toPosition returns the correct position information for the given pos, removing sandbox prefix
// if any.
func (p *primitivizer) toPosition(pos token.Pos) token.Position {
Expand Down