Skip to content

perf(semantic): conform to a named type by the element it names - #1349

Open
devin-ai-integration[bot] wants to merge 3 commits into
developfrom
perf/conform-by-symbol
Open

devin-ai-integration[bot] wants to merge 3 commits into
developfrom
perf/conform-by-symbol

Conversation

@devin-ai-integration

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

Copy link
Copy Markdown
Contributor

What and why

The 0.11.0 performance record (measured against v0.10.1) finds the connector-dense ImportValues benchmarks 29–37% more allocations per load (flat/parts=100/attrs=3 135.5k → 175.2k, nested/parts=500/attrs=3 864.9k → 1 189.0k), Analyze/synthetic +10% (4.39M → 4.83M) and WorkspaceEdit/synthetic/reindex+diagnostics +9%. Bisected over the pull-request merges to #1298 (constancy conformance on the derived variability): restrictionViolations now asks FeatureIsVariable for every redefinition, which derives Usage::mayTimeVary through four conformsByName checks (Occurrences::Occurrence, Links::SelfLink, Occurrences::HappensLink, Actions::Action), and conformsByName built the qualified name of the candidate and of each of its supertypes with Index.GetFQN on every check — about 73k of the 175k allocations of one ImportValues load (pprof: restrictionViolations → FeatureIsVariable → UsageMayTimeVary → conformsByName → FQNOf).

conformsByName now splits the asked name once and compares the candidate's and each memoized supertype's owner chain against it segment by segment with symbols.FQNIs (from #1348, on which this branch is stacked), so the comparison is the same name comparison as before and no qualified name is built. Without an index it compares local names, as fqnOf did.

Specification basis

None: the answers are unchanged; only how the name comparison is made.

How it was verified

  • TestConformsByNameDoesNotBuildNames (internal/semantic/semantics): Leaf :> Mid :> Base conforms to T::Base, T::Mid, T::Leaf and not to T::Other, the unregistered T::Absent or the root name Base, and a repeated check allocates nothing. On develop before this change the repeated checks allocate 1–3 objects each and the test fails.
  • Benchmark A/B, six interleaved rounds on the record's machine, pre-regression commit 2cfcb09df (the parent of the fix(semantic): constancy conformance reads a SysML usage's derived variability #1298 merge) in a scratch worktree | develop b60a18c20 | this branch (first version, same allocation profile), allocs/op: ImportValues/flat/parts=100/attrs=3 135.5k | 175.2k | 135.5k (B/op 11.20 | 12.03 | 11.20 MiB); ImportValues/nested/parts=500/attrs=3 864.9k | 1 189.0k | 864.9k; Analyze/synthetic 4.394M | 4.834M | 4.339M; WorkspaceEdit/synthetic/reindex+diagnostics 4.838M | 5.278M | 4.783M; FeaturesOf 5.416M in all three (its move predates 2cfcb09df and is attributed separately). sec/op within the busy machine's noise in every column.
  • go test ./internal/semantic/... ./internal/check/passes ./internal/workspace/model ./tests/resolve ./tests/perf pass, go build ./..., gofmt -l . empty, python3 scripts/changelog.py check.

Checklist

  • make test and make lint pass locally (the packages above; the full suite runs in CI)
  • Tests added or updated for the change
  • Documentation extended where it already covers the surface (no user-facing surface changes)
  • 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 (none moved)
  • 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/cf22d7a746814d20b2f0eaf11682fef9
Open in Devin Desktop: https://nasa-jpl-demo.devinenterprise.com/desktop/session/cf22d7a746814d20b2f0eaf11682fef9?variant=devin
Requested by: @HuiJun

Co-Authored-By: jason.han <hanhuijun@gmail.com>
@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.

devin-ai-integration Bot and others added 2 commits October 21, 2026 07:47
…stered element

Co-Authored-By: jason.han <hanhuijun@gmail.com>
@devin-ai-integration
devin-ai-integration Bot changed the base branch from develop to perf/enumerate-children-without-route-walks October 21, 2026 07:49

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Devin Review found 2 new potential issues.

Devin Review

Comment on lines +1005 to +1009
if symbols.FQNIs(sym, parent, leaf) {
return true
}
for _, sup := range m.AllSupertypes(sym) {
if m.fqnOf(sup) == typeFQN {
if symbols.FQNIs(sup, parent, leaf) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🔴 Imported type aliases lose conformance

When a type reaches Links::BinaryLink through a public import, conformsByName rejects its original symbol. FQNIs sees the declaration's owner chain, not the name registered by the import. Binary connector checks then return false.

Learn more

The index can register the same symbol under more than one qualified name. A wildcard public import registers it under the importing namespace, and a declared short name registers it under another name in the same namespace. symbolByFQN retrieves the element under either key; FQNIs only compares its original declaration chain. Both the candidate and its supertypes can therefore lose a conformance match that the index recognizes. IsBinaryConnector consumes this answer for connector classification.

Example: Declare Lib::BinaryLink, then package Links { public import Lib::*; }. Links::BinaryLink resolves to Lib::BinaryLink. Checking the latter against Links::BinaryLink returns false instead of true.

Recommended fix: Retain an index-aware identity lookup for names that resolve to a registered element, including imported and short-name registrations. Compare symbols.SameElement against the candidate and its supertypes; keep the allocation optimization only where it preserves that lookup's semantics.

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This matches develop and is not a regression. There, conformsByName compared m.fqnOf(sup) == typeFQN, and fqnOf is Index.GetFQN, which is symbols.FQNOf: the declaration's owner-chain name. Lib::BinaryLink checked against Links::BinaryLink already returned false on develop. FQNIs(sym, parent, leaf) is defined as FQNOf(sym) == parent::leaf (pinned by TestFQNIsMatchesFQNOf), so it gives the same answer. Only this PR's first revision (55b542a's parent), which compared against the element symbolByFQN returns, would have accepted the alias. Whether a type that reaches the library name only through an import should conform is a semantic change, not a performance one, so it is out of scope here. I'm leaving the thread open for the maintainer.

Comment on lines +1005 to +1009
if symbols.FQNIs(sym, parent, leaf) {
return true
}
for _, sup := range m.AllSupertypes(sym) {
if m.fqnOf(sup) == typeFQN {
if symbols.FQNIs(sup, parent, leaf) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🔴 Distinct namesakes falsely conform to indexed types

When an indexed type has an unindexed namesake, conformsByName accepts both by owner-chain name. symbolByFQN previously identified the indexed element, so the distinct namesake did not conform. This can misclassify types after reindexing.

Learn more

A symbol's owner-chain name does not prove that it is the element currently registered under that name. Workspace reindexing can leave callers holding a symbol from another scope tree. When the index has exactly one element under the target name, the previous lookup compared element identity and rejected a different declaration with the same spelling. FQNIs cannot distinguish those declarations, so the conformance answer changes.

Example: An older document's Links::BinaryLink symbol remains in a query while the active index registers a replacement declaration at Links::BinaryLink. The old symbol now conforms to the new declaration by name, despite representing a different element.

Recommended fix: Resolve the registered target before using the owner-chain fast path. When a unique target exists, compare symbols.SameElement for the candidate and each supertype; only use name-based comparison in the cases where the existing contract permits it.

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This matches develop too. There, conformsByName never consulted symbolByFQN. It compared fqnOf(sup) (= symbols.FQNOf, the owner-chain name) to typeFQN, so a stale or unindexed namesake with the same owner chain already conformed. FQNIs is that same comparison without building the string. The identity check described here was only in this PR's first revision, and the previous review found that it misjudges declarations that moved across index generations. That's why I went back to the name comparison. This change keeps develop's contract exactly: the same answers with fewer allocations. I'm leaving the thread open for the maintainer in case identity semantics are wanted as a separate change.

This branch has not been deployed

No deployments
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