Skip to content

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

Merged

Conversation

michaelnebel
Copy link
Contributor

@michaelnebel michaelnebel commented Sep 15, 2022

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:
image

To improve the join ordering and to keep the intermediate relations as small as possible, we use the following observations

  • When we want to use specific specs (ie. a spec that directly targets a callable), we would like to join Callable and elementSpec on three columns in one go (namespace, type and name).
  • When we want to use specs that are valid for all overrides (subtypes = true), we need project the elementSpec relation (on subtypes = true as this is relatively small subset) and join on Callable name. Afterwards we can inspect the types.

The new predicates and tuples counts can be seen here.
image
image
image

@github-actions github-actions bot added the C# label Sep 15, 2022
@michaelnebel
Copy link
Contributor Author

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).
In any case, the following DCA executions indicates that the changes on this PR solves the join order problem for InterpretedCallable:

Running DCA against main has 281 issues reported in the join order badness section (33 related to InterpretedCallable):
https://github.com/github/codeql-dca-main/blob/data/michaelnebel/main__nightly__nightly/reports/summaries/any.md#join-order-badness-per-source-and-predicate-v0

Running DCA against the branch on this PR has 248 issues reported in the join order badness secion (0 related to InterpretedCallable)
https://github.com/github/codeql-dca-main/blob/data/michaelnebel/fix-joinorder-i__nightly__nightly/reports/summaries/any.md#join-order-badness-per-source-and-predicate-v0

Also note that math says: 281 - 33 = 248

@michaelnebel michaelnebel marked this pull request as ready for review September 19, 2022 10:01
@michaelnebel michaelnebel requested a review from a team as a code owner September 19, 2022 10:01
@michaelnebel michaelnebel added the no-change-note-required This PR does not need a change note label Sep 19, 2022
Copy link
Contributor

@hvitved hvitved left a 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)
Copy link
Contributor

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)
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think you are right! However, in this particular case the join is worsened slightly.
Let us try with your solution and run DCA.

image

image

image

Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes - I totally agree.

@michaelnebel
Copy link
Contributor Author

Weird actions error on last DCA execution. Will try again.

@michaelnebel
Copy link
Contributor Author

DCA looks good
(1) InterpretedCallable has disappeared from the join order badness list.
(2) None of the newly introduced predicates have turned up on the join order badness list

@michaelnebel michaelnebel merged commit 342c876 into github:main Sep 23, 2022
@michaelnebel michaelnebel deleted the csharp/fix-joinorder-interpretedcallable branch September 23, 2022 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C# no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants