-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Java: Improve a couple of join-orders #20127
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR improves performance of CodeQL query evaluation by optimizing join orders in three specific predicates through refactoring to better guide the query optimizer's decision-making.
- Restructures
closeCalled
predicate to prevent inefficient join ordering - Refactors
getErasedRepr
to minimize cartesian product impact on optimizer estimates - Modifies
objType
predicate to use bounded transitive closure for better performance
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
CloseType.qll | Restructures closeCalled predicate logic to improve join order optimization |
ObjFlow.qll | Introduces new type system and bounded transitive closure to optimize objType predicate |
DataFlowPrivate.qll | Extracts helper predicates to reduce cartesian products in getErasedRepr |
Dca looks good (uneventful). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, two suggestions.
@@ -348,6 +348,16 @@ predicate expectsContent(Node n, ContentSet c) { | |||
FlowSummaryImpl::Private::Steps::summaryExpectsContent(n.(FlowSummaryNode).getSummaryNode(), c) | |||
} | |||
|
|||
pragma[nomagic] | |||
private predicate numericRepresentative(RefType t) { | |||
t.(BoxedType).getPrimitiveType().getName() = "double" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May as well change the type of t
to BoxedType
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, I guess. Although there's a slight point in making the type RefType
, as this predicate points out a single RefType
as the representative for numeric types. Regardless, it's quite minor (and doesn't affect RA), so I think I'll just merge as-is and move on.
|
||
pragma[nomagic] | ||
private predicate booleanRepresentative(RefType t) { | ||
t.(BoxedType).getPrimitiveType().getName() = "boolean" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same.
|
||
/** | ||
* Holds if the qualifier `n` of an `Object.toString()` call might have type `t`. | ||
*/ | ||
pragma[noopt] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice to get rid of this.
3 commits fixing 3 join-orders:
closeCalled
code was quite tangled, which caused the optimiser to selectNOT #prev
before#prev_delta
. This is always bad, so the optimiser only does this when it's pushed sufficiently into a corner - usually due to excessive nesting. A little bit of untangling helped.getErasedRepr
inherently contains cartesian products, so the optimiser can't avoid them, which messes with its estimates. A slight refactor ensures that the CPs are with predicates of size 1 as they should be.fastTC
was being nicely transformed todoublyBoundedFastTC
by the optimiser, however thesource(t, n)
join represents a step with a huge fanout, so by including that in the graph the resulting bounded TC becomes orders of magnitude smaller.Tuple counts:
1.
Before
After:
Before:
After:
Before
After: