Skip to content

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

Merged
hvitved merged 2 commits into
github:mainfrom
hvitved:dataflow/stage1-param-self-prune
Jan 8, 2025
Merged

Data flow: Prune parameter-self flow in stage 1#18333
hvitved merged 2 commits into
github:mainfrom
hvitved:dataflow/stage1-param-self-prune

Conversation

@hvitved

@hvitved hvitved commented Dec 19, 2024

Copy link
Copy Markdown
Contributor

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 AI review requested due to automatic review settings January 7, 2025 12:12

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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.

@aschackmull aschackmull left a comment

Copy link
Copy Markdown
Contributor

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

hvitved commented Jan 7, 2025

Copy link
Copy Markdown
Contributor Author

IIRC you were experimenting with elimination of the allowParameterReturnInSelf concept?

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

@aschackmull aschackmull left a comment

Copy link
Copy Markdown
Contributor

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
@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.

3 participants