-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
DataFlow: Support stateless isSink
in StateConfigSig
s
#13851
Conversation
a43823c
to
f3e3dac
Compare
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 is going to need some changes.
f3e3dac
to
8399d4f
Compare
8399d4f
to
d755b39
Compare
d755b39
to
3007fda
Compare
@aschackmull I've rebased the PR now that #13863 has been merged (🎉). |
Hmmm @aschackmull I think adding the additional conjuncts to |
@@ -3981,7 +4003,7 @@ module MakeImpl<DataFlowParameter Lang> { | |||
|
|||
private predicate relevantState(FlowState state) { | |||
sourceNode(_, state) or | |||
sinkNode(_, state) or | |||
sinkNodeWithState(_, state) or |
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.
We can make another predicate to do a best-effort cartesian approximation for the reverse flow exploration case - this doesn't have to be extremely performant as it's just a debugging tool that'll often be restricted to specific sources/sinks anyway.
Add the following just below the relevantState
predicate:
private predicate revSinkNode(NodeEx node, FlowState state) {
sinkNodeWithState(node, state)
or
Config::isSink(node.asNode()) and
relevantState(state) and
not fullBarrier(node) and
not stateBarrier(node, state)
}
and use it in the two places below.
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.
That makes sense. Thanks! Fixed in 981f675.
Co-authored-by: Anders Schack-Mulligen <[email protected]>
Co-authored-by: Anders Schack-Mulligen <[email protected]>
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.
LGTM now.
Hm, it looks like Java's DCA run isn't super happy about these changes. I'll investigate! |
It might very well be unrelated to this PR - I think we're seeing a lot of OOM kills in DCA at the moment for other reasons. |
Ah, thanks for the heads up. Stage timings also seem to blame a bunch of non-dataflow related queries so it's probably a fluke. In any case, I've started a separate run for Java, and I'll do a couple of local evaluations to make sure there's nothing wrong |
FWIW, none of the projects that reported a slowdown on DCA seems to be slowing down locally. |
Java has a bunch of OOMs, but this PR doesn't seem to contribute to any more of those OOMs. And since Java (nor any other language) isn't actually using this new feature yet this doesn't seem like it should block this PR. I've also verified that no bad joins are introduced. |
Sometimes it's necessary to have a state-based configuration to define the correct
isBarrier
, but if data then does manage to reach a sink, any state should be accepted. Prior to this PR, the only way to prevent a cartesian product would be to do something like:because there was no
isSink/1
onStateConfigSig
. With this PR we can now do:with no
PruningFlow
mess.cc @aschackmull I hope this isn't too controversial?