Skip to content

Type inference: Performance tweaks - #22523

Draft
hvitved wants to merge 3 commits into
github:mainfrom
hvitved:type-inference/perf-fixes
Draft

hvitved wants to merge 3 commits into
github:mainfrom
hvitved:type-inference/perf-fixes

Conversation

@hvitved

@hvitved hvitved commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

This PR makes a few performance improvements in the shared type inference library.

@github-actions github-actions Bot added the Rust Pull requests that update Rust code label Sep 8, 2026
@hvitved
hvitved force-pushed the type-inference/perf-fixes branch from 594dfc9 to 05cf887 Compare September 8, 2026 13:20
@hvitved
hvitved force-pushed the type-inference/perf-fixes branch from 05cf887 to d010114 Compare September 16, 2026 17:44
@github-actions github-actions Bot removed the Rust Pull requests that update Rust code label Sep 16, 2026
@hvitved
hvitved force-pushed the type-inference/perf-fixes branch from d010114 to 979ddae Compare September 18, 2026 07:55
@hvitved
hvitved requested a balanced review from Copilot September 18, 2026 08:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Dynamic regexes do not escape metacharacters in encoded type paths, potentially dropping valid inference results.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity

Open (1)
What changed in this PR

Optimizes shared type-inference matching and join ordering.

Changes:

  • Refactors missing type-argument checks.
  • Narrows relevant access targets.
  • Uses regex-based path prefix selection.
File Description
shared/​typeinference/​codeql/​typeinference/​internal/​TypeInference.qll Optimizes type matching and path resolution.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread shared/typeinference/codeql/typeinference/internal/TypeInference.qll Outdated
@hvitved
hvitved force-pushed the type-inference/perf-fixes branch 2 times, most recently from dfb9e08 to b18b617 Compare September 18, 2026 08:55
@hvitved
hvitved requested a balanced review from Copilot September 18, 2026 08:57

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

The regex implementation loses valid results when prefix candidates overlap.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 Low severity

Open (1)
Resolved since last review (1)

Comment thread shared/util/codeql/util/Strings.qll Outdated
@hvitved
hvitved force-pushed the type-inference/perf-fixes branch from b18b617 to 3720a8c Compare September 18, 2026 09:03
@aschackmull

aschackmull commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

The general solution is overly complicated. Everything is simpler if you decompose the calculation into its two parts: the inverse append and the join with the fanout predicate. First project fanout to get the set of prefixes:

predicate fanOut2(string prefix, int res) { exists(string s | prefix = s + "." and fanOut(s, res)) }
predicate prefix(string prefix) { fanOut(prefix, _) }

Then do the inverse append:

predicate invAp(string s, string prefix, string suffix) {
  strings(s) and
  prefix(prefix) and
  s = prefix + suffix
}

Then join:

predicate invApJoined(string s, int i, string suffix) {
  exists(string prefix | invAp(s, prefix, suffix) and fanOut2(prefix, i))
}

This should have the same performance characteristic as the regex-based version, since the regex version also effectively performs the projected CP of strings and prefix.

@hvitved

hvitved commented Sep 18, 2026

Copy link
Copy Markdown
Contributor Author

Everything is simpler if you decompose the calculation into its two parts: the inverse append and the join with the fanout predicate.

Right, only problem is that this doesn't work unless we bind s up-front (in my example this was easy). In my actual use-cases the strings are only bound as part of a big recursion, and constraining them via an additional input predicate would lead to two recursive calls instead of one (and be somewhat clunky).

@aschackmull

Copy link
Copy Markdown
Contributor

Right, only problem is that this doesn't work unless we bind s up-front (in my example this was easy). In my actual use-cases the strings are only bound as part of a big recursion, and constraining them via an additional input predicate would lead to two recursive calls instead of one (and be somewhat clunky).

Granted, I haven't looked at your actual use-case, but I don't see how that makes a difference. Constructing the regex is essentially equivalent to the projection to the prefix column, so if your complex solution works, then the simple ought to as well.

@aschackmull

Copy link
Copy Markdown
Contributor

Another way to phrase the simple solution is as follows: You have a big CP because the join-orderer needs to pick fanout/2 at some point, but if you replace fanout(s, i) with the equivalent fanout(s, _) and fanout(s, i) then a much better join-order becomes possible, since the use of s can be sandwich'ed between picking fanout(s, _) and fanout(s, i). In this case the use happens to be an inverse append, but that's actually irrelevant.

@hvitved

hvitved commented Sep 18, 2026

Copy link
Copy Markdown
Contributor Author

Turns out that the effect of the inverse-append changes were in fact not helping at all, and that the actual performance improvements were the other changes, so I have reverted.

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.

4 participants