Skip to content

C++: Speedup 'cpp/using-expired-stack-address' by avoiding a large ne… #10321

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

Merged

Conversation

MathiasVP
Copy link
Contributor

This should be a behavior-preserving change that removes a large antijoin from the cpp/using-expired-stack-address query that we were seeing on petsc/petsc:

Tuple counts for UsingExpiredStackAddress#38a029fe::stackAddressEscapes#4#ffff#antijoin_rhs/4@8cee18ou after 2m19s:
    500       ~0%     {4} r1 = JOIN UsingExpiredStackAddress#38a029fe::stackAddressEscapes#4#ffff#shared WITH UsingExpiredStackAddress#38a029fe::globalAddress#1#ff#reorder_1_0_10#join_rhs ON FIRST 1 OUTPUT Rhs.1 'arg3', Lhs.1 'arg0', Lhs.2 'arg1', Lhs.3 'arg2'
    899500    ~1%     {5} r2 = JOIN r1 WITH UsingExpiredStackAddress#38a029fe::globalAddress#1#ff#reorder_1_0 ON FIRST 1 OUTPUT Lhs.3 'arg2', Lhs.1 'arg0', Lhs.2 'arg1', Lhs.0 'arg3', Rhs.1
    898500    ~1%     {5} r3 = JOIN r2 WITH Instruction#577b6a83::StoreInstruction#f ON FIRST 1 OUTPUT Lhs.0 'arg2', Lhs.1 'arg0', Lhs.2 'arg1', Lhs.3 'arg3', Lhs.4
    
    897500    ~6%     {6} r4 = JOIN r3 WITH IRBlock#896e97af::IRBlockBase::getAnInstruction#0#dispred#ff_10#join_rhs ON FIRST 1 OUTPUT Rhs.1, Lhs.1 'arg0', Lhs.2 'arg1', Lhs.0 'arg2', Lhs.3 'arg3', Lhs.4
    61532500  ~0%     {6} r5 = JOIN r4 WITH boundedFastTC:IRBlock#896e97af::Cached::blockSuccessor#2#ff:UsingExpiredStackAddress#38a029fe::stackAddressEscapes#4#ffff#higher_order_body ON FIRST 1 OUTPUT Rhs.1, Lhs.1 'arg0', Lhs.2 'arg1', Lhs.3 'arg2', Lhs.4 'arg3', Lhs.5
    853293000 ~0%     {6} r6 = JOIN r5 WITH IRBlock#896e97af::IRBlockBase::getAnInstruction#0#dispred#ff ON FIRST 1 OUTPUT Rhs.1, Lhs.5, Lhs.1 'arg0', Lhs.2 'arg1', Lhs.3 'arg2', Lhs.4 'arg3'
    
    897000    ~4%     {7} r7 = JOIN r3 WITH IRBlock#896e97af::Cached::getInstruction#2#fff_201#join_rhs ON FIRST 1 OUTPUT Rhs.1, Lhs.1 'arg0', Lhs.2 'arg1', Lhs.0 'arg2', Lhs.3 'arg3', Lhs.4, Rhs.2
    11660000  ~6%     {9} r8 = JOIN r7 WITH IRBlock#896e97af::Cached::getInstruction#2#fff ON FIRST 1 OUTPUT Lhs.1 'arg0', Lhs.2 'arg1', Lhs.3 'arg2', Lhs.4 'arg3', Lhs.5, Lhs.0, Lhs.6, Rhs.1, Rhs.2
    2690500   ~3%     {9} r9 = SELECT r8 ON In.7 > In.6
    2690000   ~2%     {6} r10 = SCAN r9 OUTPUT In.8, In.4, In.0 'arg0', In.1 'arg1', In.2 'arg2', In.3 'arg3'
    
    855983000 ~0%     {6} r11 = r6 UNION r10
    0         ~0%     {4} r12 = JOIN r11 WITH Instruction#577b6a83::StoreInstruction::getDestinationAddress#0#dispred#ff ON FIRST 2 OUTPUT Lhs.2 'arg0', Lhs.3 'arg1', Lhs.4 'arg2', Lhs.5 'arg3'
                      return r12

(the tuple counts is from a run that I cancelled.)

@MathiasVP
Copy link
Contributor Author

@geoffw0 d6b8f25 adds a few more tests that you requested in #10320.

@geoffw0
Copy link
Contributor

geoffw0 commented Sep 6, 2022

Fantastic, thanks! In that case I think we just need confirmation from the DCA run.

@geoffw0
Copy link
Contributor

geoffw0 commented Sep 7, 2022

DCA:

  • we can ignore the failure of Nelson-numerical-software__nelson.
  • I'm not sure how to interpret the join-order badness results for cpp/using-expired-stack-address.
  • query run time for cpp/using-expired-stack-address is about the same.

So no regression, and we weren't really expecting an improvement on DCA as the fix is specific to a minority of projects such as petsc/petsc where this exploded.

Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@MathiasVP
Copy link
Contributor Author

I'm not sure how to interpret the join-order badness results for cpp/using-expired-stack-address.

Nothing concerning there. There were some bad joins highlighted by the metric even before this PR, but they haven't caused any issues in the wild (unlike the issue fixed by this PR). And since this is targeting the rc/3.7 PR I don't want to inflate the diff unnecessarily.

@MathiasVP MathiasVP marked this pull request as ready for review September 7, 2022 08:26
@MathiasVP MathiasVP requested a review from a team as a code owner September 7, 2022 08:26
@MathiasVP MathiasVP added the no-change-note-required This PR does not need a change note label Sep 7, 2022
@dbartol dbartol merged commit 9504455 into github:rc/3.7 Sep 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C++ 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.

3 participants