Repository navigation
perf(semantics): memoize the base feature a declaration's kind implies - #1354
Merged
Merged
Conversation
Co-Authored-By: jason.han <hanhuijun@gmail.com>
Contributor
Author
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
Co-Authored-By: jason.han <hanhuijun@gmail.com>
Co-Authored-By: jason.han <hanhuijun@gmail.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.
What and why
The CircleCI
Go race tests(build 12113) andGo coverage shards(build 12104) jobs on develop died ongo test's 30-minute per-package budget at d8b60ed, the merge of #1319 (usages subset the base library usage of their kind), and had passed on b60a18c just before. This PR measures where the time went and removes the remaining cost of that change on the hot paths.Root cause of (a). After #1319 every
ImplicitGenerals/DirectSupertypesderivation callsimplicitUsageBaseFeature,implicitKerMLFeatureBase,implicitBaseUsageanddeclaredChainReachesKindBase, each of which re-walks the declared generalization chain (declaredGeneralizationReaches) and re-resolves the library name (LookupQualified), five to eight times per symbol. Counting overtests/stressmodelon develop: 178 300computeImplicitGeneralscalls (2.66 M memo hits), none provisional, so the memo itself works — each miss is simply expensive. The fix memoizes the kind base once per symbol, journaled and guarded exactly likeimplicitBasesandImplicitGenerals; semantics and outputs are unchanged.Overlap with open PRs. None memoizes this. #1349 (
conformsByName,filter.go), #1232 (API JSON, touchesimplicit.gofor derived properties only), #1327 (crossing.go), #1314 (gather.go/model.gorecording) and the runtime PRs #1340/#1308/#1310/#1311/#1312 do not touch the kind-base lookup.Step 1 — where the time went (last 26 develop merges)
Three causes, separated:
internal/exec/runtimeexploration run 1168–1415 s on the 19 merges before → 1803 s = timeout at d8b60ed;tests/stressmodel157–190 s → 513 s (3.0×);tests/model1478–1877 s → 2189 s. Coverage job:cmd/sysml1353–1773 s → timeout;internal/exec/runtime1018–1299 s → 1671 s;tests/model853–959 s → 1373 s. Packages with no semantics-heavy tests (tests/migrate,tests/corpus,internal/exec/smt) did not move at that commit. The six develop merges after it (fix(semantics): derive a declaration's implied generals once per symbol #1350, perf(runtime): compute an expression's footprint once per model #1347, perf(resolve): enumerate imported members without walking their routes #1348, perf(runtime): compute a feature's effective types once per model #1351, perf(repl): count a diagnostic's UTF-16 column without encoding the line #1352, perf(semantic): compute a feature's redefinition closure once per model #1353 — CircleCI builds 12114–12175, run on 2026-10-21) show what the already-merged perf PRs recovered:internal/exec/runtimerace 1294–1426 s andcmd/sysmlcoverage 1511–1571 s are back in their pre-feat(semantics): usages subset the base library usage of their kind #1319 bands from fix(semantics): derive a declaration's implied generals once per symbol #1350 on (a662c2b race: 1359 s), while the semantics-heavy packages are not —tests/stressmodelrace 182–213 s against 157–190 s before (+10–28 %), coverage 113–122 s against 101 s (+12–21 %),tests/modelrace 1582–1853 s at the top of its band. That residue is what this PR removes.tests/corpusrace 633 s (feat(check): connection and flow definition ends must be usages #1301) → 1125 s (fix(lower): project a named terminate node into the transition footprint #1326) as corpus gates were added;tests/migrate1932–2383 s on a 55 m budget throughout;cmd/sysmlunder-coverpkg=./...1353–1773 s, i.e. under 2 minutes of headroom on a 30 m budget before feat(semantics): usages subset the base library usage of their kind #1319, so any regression tips it over.resource_class: large,parallelism: 5, same shard list, same cache. GitHubpr.ymlshards are too noisy to attribute (the runtime shard takes 21–41 min on unchanged code), which is why the CircleCI columns are the evidence. The develop pipelines after d8b60ed sat unrun until 2026-10-21 13:51 UTC, which is why their rows carry that date.CircleCI
Go race tests(resource_class large, parallelism 5): per-node wall minutes and per-packagego testsecondsCircleCI
Go coverage shards(large, parallelism 5,-coverpkg=./...): per-node wall minutes and per-package secondsGitHub
pr.ymlfull-tier shards on each merged PR's head:Run Go testsstep minutesStep 2 — before/after, same machine (8 vCPU, go1.25.11),
go test -count=1cmd/sysml -run TestExploreTablesTheSpacecraftRaceWithinItsBudget(packageokseconds)internal/exec/runtime -run TestExploreWithIsExploreOverTheConformanceCorpus(wall)tests/stressmodel(packageokseconds)go tool pprof -top -cum -diff_basedevelop vs b60a18c ontests/stressmodel:ImplicitGenerals+4.21 s,computeImplicitGenerals+4.04 s,implicitUsageBaseFeature+2.24 s,DirectSupertypes+1.86 s,candidateBaseFeatures+1.64 s,declaredGeneralizationReaches+1.50 s,implicitBaseFeatures+1.34 s,declaredChainReachesKindBase+0.62 s;implicitBases−1.30 s (already memoized by #1350). On the spacecraft exploration and the conformance-corpus exploration the develop-vs-before diff is GC noise plus ≤0.5 s inTimeEventConforms; the perf PRs already merged recovered those.Step 3 — CI configuration
With (a) removed, one package was still living on the budget's edge before #1319 for reasons (b)/(c):
cmd/sysmlunder-coverpkg=./... -covermode=atomictook 1353–1773 s of the 1800 s budget on every one of the 19 green coverage runs before #1319 (1773 s at #1329, 27 s of headroom) and still 1511–1571 s on the six after it with the perf PRs in, while sharing therestnode's 4 vCPUs with ~2200 s of other packages (tests/codegen709 s,internal/frontend/grpc416 s,internal/frontend/repl285 s,tests/diagnostics174 s,internal/workspace/libs141 s,cmd/sysml-grpc127 s, …). Therestshard is also the longest GitHubpr.ymlshard (25–45 min). Socmd/sysmlbecomes a shard of its own,cli:scripts/race-shard.sh:cli→cmd/sysml, whichrestno longer contains..circleci/config.yml: both jobsparallelism: 6, shard listruntime model export rest migrate cli; the node-count guard stays.Makefile:COVERAGE_SHARDSgainscli;test-shard,test-quickandcoverage-shardaccept it..github/workflows/pr.yml: full tier gains theclishard, the quick tier runs it withrest migrate, and the merge step expects 7 profiles.No
-timeoutchanged, nothing is skipped or marked short, and the node-count guards keep the three shard lists in step. CircleCI'sbuild-testworkflow is filtered tomain/develop, so theclinode first runs on the develop pipeline after this merges; on this PR the same shard layout is exercised by the GitHub full tier (Go race tests (cli),Go race coveragemerging 7 profiles).How it was verified
TestKindBaseIsLookedUpOncePerSymbol(internal/semantic/semantics): the memoized lookups answer as the first query did and a repeatedimplicitUsageBaseFeature,implicitBaseUsage,declaredChainReachesKindBaseandImplicitGeneralsquery allocates nothing.go build ./...,go vet ./...,gofmt -l .empty;go test -count=1 ./internal/semantic/semantics ./tests/stressmodel ./internal/exec/runtime ./cmd/sysml; fullgo test -count=1 ./...(race-free) locally.scripts/race-shard.sh cliprintscmd/sysml,scripts/race-shard.sh restno longer does; both workflow files parse;make -n coverage-shard SHARD=cli.go test -count=1.Go race testsruntime 34 min, runtime-corpus 34, model 23, export 31, rest 30 (withoutcmd/sysml), migrate 17, cli 9;Go race coveragemerged the 7 profiles. Devin Review: no findings.Checklist
make testandmake lintpass locally (race-freego test ./...;make lintunchanged surface)scripts/race-shard.shusage, both updatedchanges/unreleased/<slug>.<section>.md, not as an edit toCHANGELOG.mdmake docs-countsrun if a gate count moved (compliance rows need nothing: the census is counted at docs build)F4,K5) in the body, docs, or changelog