Michaelrfairhurst/compatible types performance fix alt #891
Conversation
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
…oin orders. More pragmas added to encourage the join ordering pipeline to make function comparisons more efficient. New approach in type equivalence assumes that all types are trivially equivalent to themselves. Therefore, only type comparisons between non-identical types need to be considered as interesting roots. The types that are reachable in the type graph from these roots are the ones considered by the recursive type equivalence predicate.
There was a problem hiding this comment.
Pull Request Overview
This PR introduces change notes documenting performance improvements for type comparisons in several CodeQL queries.
- Added pragmas to control join order on function parameter equivalence.
- Refactored expressions to avoid optimizer confusion that produced Cartesian products.
- Adjusted modules (Compatible.qll and SimpleAssignment.qll) to limit expensive equality checks on types.
Files not reviewed (8)
- c/cert/src/rules/DCL40-C/IncompatibleFunctionDeclarations.ql: Language not supported
- c/misra/src/rules/RULE-23-5/DangerousDefaultSelectionForPointerInGeneric.ql: Language not supported
- c/misra/src/rules/RULE-8-3/DeclarationsOfAFunctionSameNameAndType.ql: Language not supported
- c/misra/src/rules/RULE-8-3/DeclarationsOfAnObjectSameNameAndType.ql: Language not supported
- c/misra/src/rules/RULE-8-4/CompatibleDeclarationFunctionDefined.ql: Language not supported
- c/misra/src/rules/RULE-8-4/CompatibleDeclarationObjectDefined.ql: Language not supported
- cpp/common/src/codingstandards/cpp/types/Compatible.qll: Language not supported
- cpp/common/src/codingstandards/cpp/types/SimpleAssignment.qll: Language not supported
| @@ -0,0 +1,6 @@ | |||
| - `RULE-8-3`, `RULE-8-4`, `DCL40-C`, `RULE-23-5`: `DeclarationsOfAFunctionSameNameAndType.ql`, `DeclarationsOfAnObjectSameNameAndType.ql`, `CompatibleDeclarationOfFunctionDefined.ql`, `CompatibleDeclarationObjectDefined.ql`, `IncompatibleFunctionDeclarations.ql`, `DangerousDefaultSelectionForPointerInGeneric.ql`: | |||
| - Added pragmas to alter join order on function parameter equivalence (names and types). | |||
| - Refactored expression which the optimizer was confused by, and compiled into a cartesian product. | |||
There was a problem hiding this comment.
Consider rewording this line for clarity. For instance, specify that the expression was refactored to prevent the optimizer from generating an unintended cartesian product.
Suggested change
| - Refactored expression which the optimizer was confused by, and compiled into a cartesian product. | |
| - Refactored an expression to prevent the optimizer from generating an unintended cartesian product, which could lead to performance issues. The refactoring ensures that the join order is explicitly defined, avoiding ambiguity in the optimizer's behavior. |
Copilot uses AI. Check for mistakes.
| - Added pragmas to alter join order on function parameter equivalence (names and types). | ||
| - Refactored expression which the optimizer was confused by, and compiled into a cartesian product. | ||
| - Altered the module `Compatible.qll` to only perform expensive equality checks on types that are compared to a non identical other type, and those reachable from those types in the type graph. Types that are identical will trivially be considered equivalent. | ||
| - `RULE-23-5`: `DangerousDefaultSelectionForPointerInGeneric.ql`: |
There was a problem hiding this comment.
[nitpick] The RULE-23-5 changes are mentioned both in the consolidated bullet and separately. It would improve clarity to consolidate or better distinguish the separate concerns if intentional.
Suggested change
| - `RULE-23-5`: `DangerousDefaultSelectionForPointerInGeneric.ql`: |
Copilot uses AI. Check for mistakes.
|
/test-performance |
|
🏁 Beep Boop! Performance testing for this PR has been initiated. Please check back later for results. Note that the query package generation step must complete before testing will start so it might be a minute. |
|
🏁 Beep Boop! One or things failed during performance testing. Please check the release engineering repo for details. |
|
/test-performance |
|
🏁 Beep Boop! Performance testing for this PR has been initiated. Please check back later for results. Note that the query package generation step must complete before testing will start so it might be a minute. |
|
🏁 Beep Boop! Performance testing complete! See below for performance of the last 3 runs vs your PR. Times are based on predicate performance. You can find full graphs and stats in the PR that was created for this test in the release engineering repo. 🏁 Below are the slowest predicates for the last 2 releases vs this PR. |
lcartey
reviewed
Apr 27, 2025
| predicate interestedInFunctions(FunctionDeclarationEntry f1, FunctionDeclarationEntry f2) { | ||
| f1.getDeclaration() = f2.getDeclaration() and | ||
| not f1 = f2 and | ||
| f1.getDeclaration() = f2.getDeclaration() |
lcartey
reviewed
Apr 27, 2025
| b = bFun.getParameterDeclarationEntry(i).getType() | ||
| ) | ||
| } | ||
|
|
||
| predicate equalReturnTypes(FunctionDeclarationEntry f1, FunctionDeclarationEntry f2) { |
There was a problem hiding this comment.
I think you need to add interestedInFunctions(f1,f2) here - the performance logs suggest this predicate is slow.
|
|
||
| string toString() { result = this.(Type).toString() } | ||
| /** Whether two types will be compared, regardless of order (a, b) or (b, a). */ | ||
| private predicate interestedInUnordered(Type t1, Type t2) { |
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.
Description
Alternate change to also test and compare results
Change request type
.ql,.qll,.qlsor unit tests)Rules with added or modified queries
Release change checklist
A change note (development_handbook.md#change-notes) is required for any pull request which modifies:
If you are only adding new rule queries, a change note is not required.
Author: Is a change note required?
🚨🚨🚨
Reviewer: Confirm that format of shared queries (not the .qll file, the
.ql file that imports it) is valid by running them within VS Code.
Reviewer: Confirm that either a change note is not required or the change note is required and has been added.
Query development review checklist
For PRs that add new queries or modify existing queries, the following checklist should be completed by both the author and reviewer:
Author
As a rule of thumb, predicates specific to the query should take no more than 1 minute, and for simple queries be under 10 seconds. If this is not the case, this should be highlighted and agreed in the code review process.
Reviewer
As a rule of thumb, predicates specific to the query should take no more than 1 minute, and for simple queries be under 10 seconds. If this is not the case, this should be highlighted and agreed in the code review process.