Skip to content
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

Java: IPA the CFG. #711

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

aschackmull
Copy link
Contributor

This converts the control flow graph to an IPA type to give us some more options in how we model control flow. There should be no change in any query results.

@aschackmull aschackmull requested a review from a team as a code owner December 20, 2018 15:49
private newtype TNode =
TExprNode(Expr e) { hasControlFlow(e) } or
TStmtNode(Stmt s) or
TExitNode(Callable c) { exists(c.getBody()) }
Copy link
Contributor

Choose a reason for hiding this comment

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

C# also has an entry node, for symmetry.

Copy link
Contributor

Choose a reason for hiding this comment

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

C# also uses a separate IPA type for basic blocks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was aiming for minimal changes, so I'm not going to change the CFG with e.g. a new entry node here. Using a separate type for basic blocks could of course be done, but I don't think it's that important, so I think I'll leave it as is for now.

@felicitymay
Copy link
Contributor

@aschackmull - I've raised a PR with suggested text changes for the change note.

@aschackmull
Copy link
Contributor Author

I've raised a PR with suggested text changes for the change note.

Actually, you just added the commit directly to my PR without doing a separate PR against my PR.

@felicitymay
Copy link
Contributor

Many apologies. I must have managed to click the wrong button - sorry about that.

aschackmull and others added 5 commits January 7, 2019 13:12
I started writing review comments to suggest simplifications to the text/tense, but thought that this might be clearer. 

Feel free to close and simplify using alternative wording.
@aschackmull
Copy link
Contributor Author

Looks like there are some performance problems, which are likely related to QL-167.

@adityasharad adityasharad changed the base branch from master to main August 14, 2020 18:31
@adityasharad adityasharad requested a review from a team as a code owner August 14, 2020 18:31
@edoardopirovano
Copy link
Contributor

This might be worth profiling again now that https://github.com/github/semmle-code/pull/39420 is merged to see if that fixed the performance issues.

@aschackmull
Copy link
Contributor Author

It would indeed be interesting to dust off this PR and revisit it, as we have much better tools to deal with performance problems now than back then, but as far as I recall the performance problems weren't related to the many-ipa-branches issue or ipa performance on its own, but rather the problem that ipa types don't have good size estimates, which caused a bunch of cascading changes to various join-orders.

cklin pushed a commit that referenced this pull request Apr 4, 2022
Address incorrectly referenced parameter in QLdoc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants