-
Notifications
You must be signed in to change notification settings - Fork 1.7k
C#: Fix join order in InterpretedCallable characteristic predicate. #10433
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
C#: Fix join order in InterpretedCallable characteristic predicate. #10433
Conversation
It looks like DCA is somewhat broken ATM when it comes to reporting some of the data for join order badness (we can't see the detail of a specific join order problem, only that there might be one). Running DCA against main has 281 issues reported in the join order badness section (33 related to InterpretedCallable): Running DCA against the branch on this PR has 248 issues reported in the join order badness secion (0 related to InterpretedCallable) Also note that math says: 281 - 33 = 248 |
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.
First join-fix LGTM, I think the second can be improved.
|
||
pragma[nomagic] | ||
private predicate subtypeSpecCandidate(Callable c, UnboundValueOrRefType t) { | ||
elementSpec(_, _, true, c.getName(), _, _, t) |
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.
This looks like a potentially bad join, as we will relate all calls with matching names. How about first joining on getASubTypeUnbound+()
instead?
pragma[nomagic]
private predicate subtypeSpecCandidate(string name, UnboundValueOrRefType t) {
exists(UnboundValueOrRefType t0 |
elementSpec(_, _, true, name, _, _, t0) and
t = t0.getASubTypeUnbound+()
)
}
pragma[nomagic]
private predicate callableInfo(Callable c, string name, UnboundValueOrRefType decl) {
name = c.getName() and
decl = c.getDeclaringType()
}
and then use it in InterpretedCallable
as
exists(string name, UnboundValueOrRefType t |
callableInfo(this, name, t) and
subtypeSpecCandidate(name, t)
)
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.
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.
However, in this particular case the join is worsened slightly.
I think that is only because we are currently lucky that elementSpec(_, _, true, name, _, _, t)
is rather small. But even so, if, say, elementSpec(_, _, "ToString", _, _, t)
held, we would join with all ToString
methods, which can be a large set.
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.
Yes - I totally agree.
Weird actions error on last DCA execution. Will try again. |
DCA looks good |
The original solution joined on Callable and elementSpec based on name, which lead to a quite large intermediate relation.
The tuples counts in all the screenshots below are based on the rappayne_webgoat database.
The tuple counts for the original solution can be seen here:

To improve the join ordering and to keep the intermediate relations as small as possible, we use the following observations
The new predicates and tuples counts can be seen here.


