perf: stop walking the package graph once per route - #31
Closed
nccapo wants to merge 1 commit into
Closed
Conversation
packages.Visit allocates and sorts a key slice for every package it walks, on every call. Five places called it once per route to answer one question — which analyzed package was type-checked into this types.Package — and together they were the largest source of allocation in the analyzer: keySlice alone was 46% of every object allocated, and Visit as a whole 49%. The walk could not succeed for a dependency anyway. Resolving a declaration needs syntax, and only the analyzed packages are parsed; dependencies are type-checked from export data and have none. So the scan is over the analyzed packages directly, which is also the set the credential scan and helper tracing are already confined to. On an 800-route service: analysis drops from 265ms to 116ms, allocated bytes per run from 103MB to 66MB, and objects from 645k to 487k. End to end with the previous change, 2.7x to 3.4x faster than master. Output is identical across all 23 corpora, the upstream RealWorld repository and three synthetic services. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Finding
This came out of profiling after #30, while checking whether a Go 1.27 toolchain bump would help. It won't help much — but the allocation profile pointed straight at us:
packages.Visit→keySliceresolver.findFuncDeclpipeline.findInfoForFuncDeclpackages.Visitallocates and sorts a key slice for every package it walks, on every call. Five places called it once per route to answer a single question: which analyzed package was type-checked into thistypes.Package?Change
The walk could never succeed for a dependency in the first place. Resolving a declaration needs syntax, and after #30 only the analyzed packages are parsed — dependencies are type-checked from export data and have none. So the scan goes over the analyzed packages directly.
Five call sites, in
resolver(2),auth(2) andcontract(2, one of which shared a helper), now go through a smallpackageWithTypesin each package. That set is also exactly what the credential scan and helper tracing are already confined to, so this narrows nothing that was reachable before.Results
analyze, best of 5,/usr/bin/time -lfor RSS:Isolating the analysis phase on the 800-route service (total minus load):
Peak RSS on upstream: 633 MB on master → 45 MB.
Correctness
analyze --jsonoutput is identical to #30 across all 23 testdata corpora, the upstream RealWorld repository, and three synthetic services (24 / 192 / 800 routes).go build,go vet,go test ./...,golangci-lintand the pinned upstream corpus gate all pass.Still open
pipeline.findInfoForFuncDeclandresolver.findFuncDeclstill scan every declaration of every analyzed package per route — now cheaply, since the allocation is gone, but still O(routes × declarations). Amap[token.Pos]built once per run would make both O(1). That is a shared-index refactor across four packages and belongs on its own; the allocation fix here was the part that did not need it.🤖 Generated with Claude Code