Skip to content

fix(semantics): derive a declaration's implied generals once per symbol - #1350

Merged
HuiJun merged 2 commits into
developfrom
fix/implicit-generals-memo
Oct 21, 2026
Merged

HuiJun merged 2 commits into
developfrom
fix/implicit-generals-memo

Conversation

@devin-ai-integration

@devin-ai-integration devin-ai-integration Bot commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

What and why

Develop's CircleCI run of d8b60ed (the merge of #1319) failed "Go race tests" and "Go coverage shards" on 30-minute go test timeouts: internal/exec/runtime under -race went from 1322 s (b60a18c) to over 1800 s, its coverage pass from 1192 s to 1671 s, and cmd/sysml from 1524 s to over 1800 s. Until that run is green, no SonarCloud scan of develop's head runs.

The cause is Model.ImplicitGenerals, which #1319 put on the hot path: FeatureTypeSet and featureTypes now call it where they called the memoized implicitBases, but ImplicitGenerals itself was derived afresh on every call, each time walking the declared generalization chain (declaredGeneralizationReaches) for three candidate base features. On TestExecutionConformance that walk went from 0.6 s to 7.2 s of CPU, a quarter of the run.

ImplicitGenerals now memoizes per symbol exactly as implicitBases does (m.resolver.Enter()/Leave() guard, journal for invalidation, a new implicitGenerals table); the derivation is computeImplicitGenerals, unchanged. Recorded library symbols still answer from their facts without the table.

Measured on this branch, TestExecutionConformance: 9.5 s at b60a18c, 11.3 s at d8b60ed, 8.2 s with the fix; declaredGeneralizationReaches is back to 0.8 s. The whole internal/exec/runtime package (no race) on this VM: 271 s at d8b60ed, 223 s with the fix.

How it was verified

gofmt -l . empty, go build ./..., go vet ./..., make lint; go test of internal/semantic/..., internal/workspace/model, internal/workspace/libs, internal/check/..., tests/semantics, tests/resolve, tests/hygiene and the full internal/exec/runtime package pass. TestImplicitGeneralsAreDerivedOncePerSymbol pins the memoization (a second query answers from the first's slice). CPU profiles before/after as above.

Checklist

  • make test and make lint pass locally
  • Tests added or updated for the change
  • Documentation extended where it already covers the surface (see CONTRIBUTING.md)
  • Changelog entry added as changes/unreleased/<slug>.<section>.md, not as an edit to CHANGELOG.md
  • baselines regenerated and make docs-counts run if a gate count moved (compliance rows need nothing: the census is counted at docs build)
  • No internal work-item labels (waves, slices, F4, K5) in the body, docs, or changelog

Link to Devin session: https://nasa-jpl-demo.devinenterprise.com/sessions/c2b1ac4ad6de4d8788e78fd86c7c1729
Open in Devin Desktop: https://nasa-jpl-demo.devinenterprise.com/desktop/session/c2b1ac4ad6de4d8788e78fd86c7c1729?variant=devin
Requested by: @HuiJun

Overlap with the open perf PRs

Dry-merged against the current heads of #1347 (perf/memoize-expression-footprints, 2cff947), #1348 (perf/enumerate-children-without-route-walks, 22a1a46) and #1349 (perf/conform-by-symbol, 55b542a, stacked on #1348): all three merge clean with no textual conflict, and the merged trees build, pass go vet on internal/semantic/... and internal/exec/runtime, and pass the model tests. No merge commit was needed; if one of those heads moves onto the same lines before this merges, the resolution is a merge of that head, not a redo of its change.

@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".

  • Disable automatic comment, CI, and merge conflict monitoring

devin-ai-integration[bot]

This comment was marked as resolved.

Co-Authored-By: jason.han <hanhuijun@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant