Optimize out empty ifs #9314
Optimize out empty ifs #9314
Conversation
If an if statement has no true or false sub-statements, there is no point including the if statement in the LIR graph. So instead of constructing a sub-graph for the if statement — which would include an empty if vertex and nothing else — and returning it, we return an empty graph object.
|
@andrewvc I realize this implementation isn't exactly what you had in mind (i.e. to create an My thinking behind the implementation in this PR was: why create empty Thoughts? |
|
@andrewvc for what it's worth, I'm with @ycombinator here. The |
|
LGTM btw :) |
|
I'm OK with this approach for now since this is our only optimization. I do think that this is not, however, scale-able if we add additional frontends. However, this is a relatively simple optimization, and it's literally our only real one at this point, so I'm OK with it. |
There was a problem hiding this comment.
Code LGTM. Great work :)
Could use some more tests. Those tests should probably have already existed before this PR. However, this feels like a good place to put the campsite rule (leave the code better than you found it) into effect, and round out the IfStatement tests.
| public class IfStatementTest { | ||
|
|
||
| @Test | ||
| public void testEmptyIfStatement() throws InvalidIRException { |
There was a problem hiding this comment.
I realize that this file didn't exist previously. We primarily tested this path through other means.
That said, this feels a bit incomplete with only the test for the new case we've added.
Would you mind adding one more test for the case where the if statement does have non-noop paths?
There was a problem hiding this comment.
No problem at all, happy to add more tests here!
|
Thanks for adding the comprehensive tests! I apologize if this is too nitpicky, but I do think these tests could be more concise / more accurate. I opened ycombinator#1 to show how you might do that for one of the tests. The nice thing about this approach is that since it diffs the whole graph it's a more comprehensive test than just counting things. It's also shorter to boot. |
Refactor test to use graph equality check for conciseness / accuracy
ycombinator
force-pushed
the
optimize-out-empty-ifs
branch
from
20c7553
to
cd5a9b7
Compare
|
@andrewvc Merged your PR and updated the rest of the unit tests to use the same pattern. Ready for review again. Thanks! |
There was a problem hiding this comment.
LGTM. Congrats on your first PR to Logstash Core @ycombinator ! 🎊
If an if statement has no true or false sub-statements, there is no point including the if statement in the LIR graph. So instead of constructing a sub-graph for the if statement — which would include an empty if vertex and nothing else — and returning it, we return an empty graph object. Fixes #9314
Resolves #9301.
If an if statement has no true or false sub-statements, there is no point including the if statement in the LIR graph. So instead of constructing a sub-graph for the if statement — which would include an empty if vertex and nothing else — and returning it, we return an empty graph object.