C#: Data flow performance improvements #1058
Merged
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
Before this change, ``` flowOutOfCallableStep(CallNode call, ReturnNode ret, OutNode out, CallContext cc) ``` would compute all combinations of call sites `call` and returned expressions `ret` up front. Now, we instead introduce explicit return nodes, so each callable has exactly one return node (as well as one for each `out`/`ref` parameter). There is then local flow from a returned expression to the relevant return node, and `flowOutOfCallableStep()` computes combinations of call sites and return nodes. Not only does this result in better performance, it also makes `flowOutOfCallableStep()` symmetric to `flowIntoCallableStep()`, where each argument is mapped to a parameter, and not to all reads of that parameter.
calumgrant
reviewed
Mar 8, 2019
| @@ -1100,7 +1125,10 @@ module DataFlow { | |||
| config.isSource(node) or | |||
| jumpStep(_, node) or | |||
| node instanceof ParameterNode or | |||
| node instanceof OutNode | |||
| node instanceof OutNode or | |||
| node instanceof NormalReturnNode or | |||
There was a problem hiding this comment.
Just node instanceof ReturnNode ?
| node instanceof ReturnNode | ||
| or | ||
| exists(ReturnNode rn | localFlowStep(node, rn, config) | | ||
| rn instanceof NormalReturnNode or |
There was a problem hiding this comment.
I don't see how the body of this exists can be false? So perhaps
localFlowStep(node, any(ReturnNode rn), config)
|
|
||
| override DotNet::Callable getEnclosingCallable() { this = TNormalReturnNode(result) } | ||
|
|
||
| override string toString() { result = "return" } |
There was a problem hiding this comment.
"return " + getEnclosingCallable().getName()
Could that be useful?
| @@ -591,18 +581,47 @@ module DataFlow { | |||
| abstract private class ReturnNode extends Node { } | |||
There was a problem hiding this comment.
Could it be useful to have getAReturnedExpr() on this class?
There was a problem hiding this comment.
It is an internal class, so I think we should only add it if we need it later on.
hvitved
force-pushed
the
csharp/dataflow/performance
branch
from
631c7b0 to
8959d52
Compare
calumgrant
previously approved these changes
Mar 11, 2019
calumgrant
approved these changes
Mar 11, 2019
The new report is ready (internal link), and things still look good. |
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.
This PR has a quite positive impact on performance (internal link), as the default LGTM suite contains many data-flow-based queries.
Commit-by-commit review is encouraged.