Python: add source nodes #4828
Merged
Python: add source nodes #4828
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
This fixes the major performance problem with type tracking on some (pathological) databases. The interface could probably be improved a bit. In particular, I'm thinking that we might want to have `DataFlow::exprNode` return a `LocalSourceNode` so that a cast isn't necessary in order to use `flowsTo`. I have added two `cached` annotations. The one on `flowsTo` is crucial, as performance regresses without it. The one on `simpleLocalFlowStep` may not be needed, but Java has a similar annotation, and to me it makes sense to have this relation cached.
This is only _really_ expensive when there are a _lot_ of strings in the database, but for this case, where we're always extracting the same substring of the string, it's easier -- and faster -- to just make a substring operation directly.
Since the number of relevant attributes in the `re` module is fairly small, it made sense to factor this out in a separate predicate, and the join order also became more sensible.
Here, `context.appliesTo(n)` was being distributed across all of the disjuncts, which caused poor performance. The new helper predicate, `literal_node_class` should be fairly small, since it only applies to a subset of `ControlFlowNode`s, and only assigns a limited set of `ClassObjectInternal`s to these nodes.
Also fixes a bug ("`B`" was not recognised as a bytestring prefix).
The basic idea behind this fix is that the set of possible prefixes is
fairly small, so it's easier just to precompute them, and then join
them with the entire prefix of the string in question (rather than
look at each string in isolation, get its prefix, and _then_ check
whether it looks like it's a unicode string prefix, which essentially
is what the code did before).
This has no impact on performance, but it cleans up the code a bit, and (hopefully) makes it more readable.
…aFlowPrivate.qll` Co-authored-by: yoff <lerchedahl@gmail.com>
…odeql into tausbn-python-add-source-nodes
This is against the philosophy, but we have still restricted attributes. We use this PR to test performance.
This also reverts the previous commit. It should be squashed with that one, but for now we keep the history, so we can track the performance tests.
tausbn
approved these changes
Jan 5, 2021
Contributor
tausbn
left a comment
There was a problem hiding this comment.
Looks good to me, and tests and performance seem good as well. Merging.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
These are my experiments with source nodes, based on #4622.
Testing current performance here: Job, results
The performance gains are similar to what @tausbn achieved, but we should also check the pathological snapshots.
Differences job tuning on projects that previously timed out: Job
That job result was not illuminating to me. But I could locally run the sql injection query on FreeCAD in about 100 minutes (6400.015 seconds).
Differences job for import change: Job