Rust: Fix performance issue with additionalExternalFile#21671
Rust: Fix performance issue with additionalExternalFile#21671geoffw0 wants to merge 2 commits intogithub:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Improves Rust CodeQL analysis performance by adjusting how File.fromSource() checks additionalExternalFile, addressing a regression introduced by the neutral models work (PR #21271).
Changes:
- Adds a
pragma[nomagic]wrapper predicate forgetRelativePath(). - Updates
fromSource()to call the wrapper when checkingadditionalExternalFile(...).
Show a summary per file
| File | Description |
|---|---|
| rust/ql/lib/codeql/files/FileSystem.qll | Adjusts File.fromSource() path lookup to avoid a performance regression during analysis. |
Copilot's findings
- Files reviewed: 1/1 changed files
- Comments generated: 0
| predicate fromSource() { | ||
| exists(ExtractorStep s | s.getAction() = "Extract" and s.getFile() = this) and | ||
| not additionalExternalFile(this.getRelativePath()) | ||
| not additionalExternalFile(this.getRelativePath0()) |
There was a problem hiding this comment.
Would it work if you instead do
pragma[nomagic]
private string isAdditionalExternalFile() { additionalExternalFile(this.getRelativePath()) }and then call not this.isAdditionalExternalFile() here?
There was a problem hiding this comment.
To explain a bit as to why Tom is suggesting this: When the compiler encounters a negation it needs to generate an antijoin which subtracts something from the pipeline. That something must be a materialized relation. So when you do not additionalExternalFile(this.getRelativePath()) the evaluator will:
- Evaluate
additionalExternalFile(this.getRelativePath()) - Materialize that relation (so it can be used in an antijoin)
- Generate an antijoin to subtract this materialized relation from the pipeline
With your change, the only thing that changes is that we now evaluate and materialize additionalExternalFile(this.getRelativePath0()) instead. Is that better? From your 3% comment it sounds like it is, but there's no guarantee that it is better 🤷 There can still be bad magic creeping into the materialized relation.
With Tom's suggestion the evaluator:
- Evaluates the (non-magic'd!)
isAdditionalExternalFile - Materialize it (we have to because it's a non-magic'd predicate. So it cannot be inlined)
- Generate an antijoin to subtract this materialized relation from the pipeline
Now the entire materialized body of the negation is guaranteed to not contain magic
There was a problem hiding this comment.
It's definitely better as it is, but what you propose may well be more robust. I'll switch to that and run DCA again to confirm.
Fix performance issue with
File.fromSourcesadditionalExternalFile.