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

Data flow: Prune parameter-self flow in stage 1 #18333

Merged
merged 2 commits into from
Jan 8, 2025

Conversation

hvitved
Copy link
Contributor

@hvitved hvitved commented Dec 19, 2024

Example tuple counts (~10 % reduction in stage 1):

Before

# n stage nodes fields conscand states tuples calledges tfnodes tftuples
1 10 1 Fwd 3,530,293 37,778 -1 1 5,111,856 -1 -1 -1
2 15 1 Rev 2,195,396 29,094 -1 1 3,366,038 2,078,437 -1 -1
3 20 2 Fwd 668,068 5,322 6,521 1 1,291,454 516,310 0 0
4 25 2 Rev 351,997 3,170 3,673 1 526,910 125,603 0 0
5 30 3 Fwd 19,865 75 89 1 27,492 6,212 1,108 2,127

After

# n stage nodes fields conscand states tuples calledges tfnodes tftuples
1 10 1 Fwd 3,460,957 37,591 -1 1 4,764,986 -1 -1 -1
2 15 1 Rev 2,135,520 28,847 -1 1 2,992,383 2,042,022 -1 -1
3 20 2 Fwd 654,633 5,189 6,316 1 1,274,172 509,370 0 0
4 25 2 Rev 349,487 3,144 3,641 1 522,787 124,824 0 0
5 30 3 Fwd 19,803 74 88 1 27,397 6,194 1,103 2,122

@hvitved hvitved force-pushed the dataflow/stage1-param-self-prune branch 2 times, most recently from 1837862 to 5cef673 Compare December 20, 2024 12:04
@hvitved hvitved force-pushed the dataflow/stage1-param-self-prune branch from 5cef673 to 06ba814 Compare January 6, 2025 12:23
@hvitved hvitved added the no-change-note-required This PR does not need a change note label Jan 7, 2025
@hvitved hvitved marked this pull request as ready for review January 7, 2025 12:12
@Copilot Copilot bot review requested due to automatic review settings January 7, 2025 12:12

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 (2)
  • shared/dataflow/codeql/dataflow/internal/DataFlowImpl.qll: Language not supported
  • shared/dataflow/codeql/dataflow/internal/DataFlowImplCommon.qll: Language not supported

Tip: Copilot code review supports C#, Go, Java, JavaScript, Markdown, Python, Ruby and TypeScript, with more languages coming soon. Learn more

@hvitved hvitved requested a review from aschackmull January 7, 2025 12:20
Comment on lines 597 to 600
pragma[nomagic]
private predicate fwdFlowInParam(DataFlowCall call, ParamNodeEx p, Cc cc) {
fwdFlowIn(call, _, cc, p)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The columns call and p ought to functionally determine arg on almost all cases, so I think it's better to inline this projection rather than materialising additional tuples.

Copy link
Contributor

@aschackmull aschackmull left a comment

Choose a reason for hiding this comment

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

IIRC you were experimenting with elimination of the allowParameterReturnInSelf concept? Of course, if we're not doing that then this change LGTM (modulo one minor comment).

@hvitved
Copy link
Contributor Author

hvitved commented Jan 7, 2025

IIRC you were experimenting with elimination of the allowParameterReturnInSelf concept?

Yes, I was, but performance turned out to be terrible :-(

Copy link
Contributor

@aschackmull aschackmull left a comment

Choose a reason for hiding this comment

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

LGTM

@hvitved hvitved merged commit 96bf81a into github:main Jan 8, 2025
33 checks passed
@hvitved hvitved deleted the dataflow/stage1-param-self-prune branch January 8, 2025 08:16
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