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

Dataflow: Another minor join-order fix #18512

Merged
merged 2 commits into from
Jan 17, 2025

Conversation

aschackmull
Copy link
Contributor

The first commit tweaks a join-order slightly: We can add the final column of readStepFwd to the join as a noop, but then ap2 = apCons(c, ap1) becomes implied and can be dropped, which simplifies things. This was mainly motivated by the second commit to get a cleaner RA diff.

The second commit fixes an actual join-order issue in S6Graph::localStep/10.
Before:

[2025-01-16 10:31:51] Evaluated non-recursive predicate DataFlowImpl::MakeImpl<Location::Location,DataFlowImplSpecific::JavaDataFlow>::Impl<SensitiveLoggingQuery::SensitiveLoggerFlow::C>::S6Graph::localStep/10#eaf7e0b3@d7e3e562 in 46ms (size: 49487).
Evaluated relational algebra for predicate DataFlowImpl::MakeImpl<Location::Location,DataFlowImplSpecific::JavaDataFlow>::Impl<SensitiveLoggingQuery::SensitiveLoggerFlow::C>::S6Graph::localStep/10#eaf7e0b3@d7e3e562 with tuple counts:
           63139   ~2%    {8} r1 = SCAN `num#DataFlowImpl::MakeImpl<Location::Location,DataFlowImplSpecific::JavaDataFlow>::Impl<SensitiveLoggingQuery::SensitiveLoggerFlow::C>::S6Graph::TPathNodeMid#2a7e4aad` OUTPUT In.0, In.4, In.5, In.6, In.1, In.2, In.3, In.7
            3680   ~0%    {10}    | JOIN WITH `DataFlowImpl::MakeImpl<Location::Location,DataFlowImplSpecific::JavaDataFlow>::Impl<SensitiveLoggingQuery::SensitiveLoggerFlow::C>::Stage6::fwdFlowRead/12#00c1fd2f_0123910115678#join_rhs` ON FIRST 7 OUTPUT Lhs.7, Rhs.7, Lhs.4, Lhs.5, Lhs.6, Rhs.8, Rhs.9, Rhs.10, _, _
            3680   ~0%    {10}    | REWRITE WITH Out.8 := "", Out.9 := false
                      
           63139   ~5%    {8} r2 = SCAN `num#DataFlowImpl::MakeImpl<Location::Location,DataFlowImplSpecific::JavaDataFlow>::Impl<SensitiveLoggingQuery::SensitiveLoggerFlow::C>::S6Graph::TPathNodeMid#2a7e4aad` OUTPUT In.5, In.0, In.1, In.2, In.3, In.4, In.6, In.7
        12371270   ~0%    {10}    | JOIN WITH `DataFlowImpl::MakeImpl<Location::Location,DataFlowImplSpecific::JavaDataFlow>::Impl<SensitiveLoggingQuery::SensitiveLoggerFlow::C>::AccessPath.isCons/2#dispred#830d3217_201#join_rhs` ON FIRST 1 OUTPUT Lhs.1, Lhs.5, Lhs.0, Lhs.6, Rhs.2, Lhs.2, Lhs.3, Lhs.4, Lhs.7, Rhs.1
            1915   ~0%    {10}    | JOIN WITH `DataFlowImpl::MakeImpl<Location::Location,DataFlowImplSpecific::JavaDataFlow>::Impl<SensitiveLoggingQuery::SensitiveLoggerFlow::C>::Stage6::fwdFlowStore/11#24237f6c_012348910567#join_rhs` ON FIRST 8 OUTPUT Lhs.8, Rhs.10, Lhs.5, Lhs.6, Lhs.7, Rhs.8, Lhs.9, Rhs.9, _, _
            1915   ~1%    {10}    | REWRITE WITH Out.8 := "", Out.9 := true
            ...

After:

[2025-01-16 12:10:51] Evaluated non-recursive predicate DataFlowImpl::MakeImpl<Location::Location,DataFlowImplSpecific::JavaDataFlow>::Impl<SensitiveLoggingQuery::SensitiveLoggerFlow::C>::S6Graph::localStep/10#eaf7e0b3@93954fc8 in 6ms (size: 49487).
Evaluated relational algebra for predicate DataFlowImpl::MakeImpl<Location::Location,DataFlowImplSpecific::JavaDataFlow>::Impl<SensitiveLoggingQuery::SensitiveLoggerFlow::C>::S6Graph::localStep/10#eaf7e0b3@93954fc8 with tuple counts:
        63139   ~0%    {8} r1 = SCAN `num#DataFlowImpl::MakeImpl<Location::Location,DataFlowImplSpecific::JavaDataFlow>::Impl<SensitiveLoggingQuery::SensitiveLoggerFlow::C>::S6Graph::TPathNodeMid#2a7e4aad` OUTPUT In.0, In.4, In.5, In.6, In.1, In.2, In.3, In.7
                   
         3680   ~0%    {10} r2 = JOIN r1 WITH `DataFlowImpl::MakeImpl<Location::Location,DataFlowImplSpecific::JavaDataFlow>::Impl<SensitiveLoggingQuery::SensitiveLoggerFlow::C>::Stage6::fwdFlowRead/12#00c1fd2f_0123910115678#join_rhs` ON FIRST 7 OUTPUT Lhs.7, Rhs.7, Lhs.4, Lhs.5, Lhs.6, Rhs.8, Rhs.9, Rhs.10, _, _
         3680   ~0%    {10}    | REWRITE WITH Out.8 := "", Out.9 := false
                   
         1920   ~0%    {9} r3 = JOIN r1 WITH `DataFlowImpl::MakeImpl<Location::Location,DataFlowImplSpecific::JavaDataFlow>::Impl<SensitiveLoggingQuery::SensitiveLoggerFlow::C>::Stage6::fwdFlowStore/11#24237f6c_012389104567#join_rhs` ON FIRST 7 OUTPUT Rhs.7, Lhs.2, Lhs.4, Lhs.5, Lhs.6, Lhs.7, Rhs.8, Rhs.9, Rhs.10
         1915   ~0%    {10}    | JOIN WITH `DataFlowImpl::MakeImpl<Location::Location,DataFlowImplSpecific::JavaDataFlow>::Impl<SensitiveLoggingQuery::SensitiveLoggerFlow::C>::AccessPath.isCons/2#dispred#830d3217_120#join_rhs` ON FIRST 2 OUTPUT Lhs.5, Lhs.8, Lhs.2, Lhs.3, Lhs.4, Lhs.6, Rhs.2, Lhs.7, _, _
         1915   ~0%    {10}    | REWRITE WITH Out.8 := "", Out.9 := true
         ...

@Copilot Copilot bot review requested due to automatic review settings January 16, 2025 12:10

Choose a reason for hiding this comment

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

Copilot wasn't able to review any files in this pull request.

Files not reviewed (1)
  • shared/dataflow/codeql/dataflow/internal/DataFlowImpl.qll: Language not supported

Tip: If you use Visual Studio Code, you can request a review from Copilot before you push from the "Source Control" tab. Learn more

Copy link
Contributor

@hvitved hvitved left a comment

Choose a reason for hiding this comment

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

Let's maybe do a DCA run for all languages (incl. JS) before merging.

@aschackmull aschackmull added the no-change-note-required This PR does not need a change note label Jan 16, 2025
@aschackmull
Copy link
Contributor Author

Dca is uneventful, merging.

@aschackmull aschackmull merged commit 498bfd2 into github:main Jan 17, 2025
32 of 33 checks passed
@aschackmull aschackmull deleted the dataflow/join-fix2 branch January 17, 2025 08:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DataFlow Library 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