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

Go: Fix bad join order in SummarizedParameterNode.gettype #17748

Merged

Conversation

owen-mc
Copy link
Contributor

@owen-mc owen-mc commented Oct 13, 2024

Specifically the disjunct for this.getPos() != -1. Running on uber/aresdb, before we had this:

   2403   ~1%    {3} r6 = JOIN `DataFlowUtil::SummarizedParameterNode.getPos/0#dispred#70a2aab4` WITH `DataFlowPrivate::FlowSummaryNode.getSummarizedCallable/0#dispred#e79ea9be` ON FIRST 1 OUTPUT Lhs.1, Lhs.0, Rhs.1
9149774   ~5%    {4}    | JOIN WITH `Types::SignatureType.getParameterType/1#dispred#2c11bb7b_102#join_rhs` ON FIRST 1 OUTPUT Lhs.2, Rhs.1, Lhs.1, Rhs.2
    923   ~9%    {2}    | JOIN WITH `Scopes::Callable.getType/0#dispred#55a0e6a2` ON FIRST 2 OUTPUT Lhs.2, Lhs.3

We add a binding pragma to make it not bind on this.getPos() until necessary. After we have this:

   2403   ~0%    {3} r6 = JOIN `DataFlowUtil::SummarizedParameterNode.getPos/0#dispred#70a2aab4` WITH `DataFlowPrivate::FlowSummaryNode.getSummarizedCallable/0#dispred#e79ea9be` ON FIRST 1 OUTPUT Rhs.1, Lhs.0, Lhs.1
   2373   ~0%    {3}    | JOIN WITH `Scopes::Callable.getType/0#dispred#55a0e6a2` ON FIRST 1 OUTPUT Rhs.1, Lhs.2, Lhs.1
    923   ~9%    {2}    | JOIN WITH `Types::SignatureType.getParameterType/1#dispred#2c11bb7b` ON FIRST 2 OUTPUT Lhs.2, Rhs.2

Specifically the disjunct for this.getPos() != -1. Running on
uber/aresdb, before we had this:

   2403   ~1%    {3} r6 = JOIN `DataFlowUtil::SummarizedParameterNode.getPos/0#dispred#70a2aab4` WITH `DataFlowPrivate::FlowSummaryNode.getSummarizedCallable/0#dispred#e79ea9be` ON FIRST 1 OUTPUT Lhs.1, Lhs.0, Rhs.1
9149774   ~5%    {4}    | JOIN WITH `Types::SignatureType.getParameterType/1#dispred#2c11bb7b_102#join_rhs` ON FIRST 1 OUTPUT Lhs.2, Rhs.1, Lhs.1, Rhs.2
    923   ~9%    {2}    | JOIN WITH `Scopes::Callable.getType/0#dispred#55a0e6a2` ON FIRST 2 OUTPUT Lhs.2, Lhs.3

We add a binding pragma to make it not bind on this.getPos() until
necessary. After we have this:

   2403   ~0%    {3} r6 = JOIN `DataFlowUtil::SummarizedParameterNode.getPos/0#dispred#70a2aab4` WITH `DataFlowPrivate::FlowSummaryNode.getSummarizedCallable/0#dispred#e79ea9be` ON FIRST 1 OUTPUT Rhs.1, Lhs.0, Lhs.1
   2373   ~0%    {3}    | JOIN WITH `Scopes::Callable.getType/0#dispred#55a0e6a2` ON FIRST 1 OUTPUT Rhs.1, Lhs.2, Lhs.1
    923   ~9%    {2}    | JOIN WITH `Types::SignatureType.getParameterType/1#dispred#2c11bb7b` ON FIRST 2 OUTPUT Lhs.2, Rhs.2
@owen-mc owen-mc added the no-change-note-required This PR does not need a change note label Oct 13, 2024
@owen-mc owen-mc requested a review from a team as a code owner October 13, 2024 13:32
@github-actions github-actions bot added the Go label Oct 13, 2024
@owen-mc
Copy link
Contributor Author

owen-mc commented Oct 15, 2024

The number of bad join orders identified by DCA goes down from 41 to 11. There is no noticeable effect on performance, so the main effect is making sure this doesn't take a long time on a pathological example.

@owen-mc owen-mc merged commit 1626af0 into github:main Oct 15, 2024
14 checks passed
@owen-mc owen-mc deleted the go/join-order-fix/data-flow-node-gettype branch October 15, 2024 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Go no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants