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: Cache TNodeEx #17300

Merged
merged 5 commits into from
Sep 24, 2024
Merged

Data flow: Cache TNodeEx #17300

merged 5 commits into from
Sep 24, 2024

Conversation

hvitved
Copy link
Contributor

@hvitved hvitved commented Aug 26, 2024

This PR moves the TNodeEx newtype from DataFlowImpl.qll into DataFlowImplCommon.qll (and makes it cached). The purpose of doing this is to avoid an additional newtype creation for all instantiations of the data flow library, and as witnessed by the DCA runs it has a very positive impact on memory/cache usage (and in general also a modest impact on timing).

Before moving the newtype, the first commit removes the Boolean hasRead column from TNodeImplicitRead, in order to the size of the newtype when later caching it. This commit then also adjusts the generated edges relation by never revealing the implicit reads that happen at sinks.

Commit-by-commit review is suggested.

@github-actions github-actions bot added the C++ label Aug 27, 2024
@hvitved hvitved force-pushed the dataflow/node-ex-cached branch 7 times, most recently from 36c92e2 to cb57b7f Compare September 2, 2024 10:47
@hvitved hvitved added the no-change-note-required This PR does not need a change note label Sep 3, 2024
@hvitved hvitved marked this pull request as ready for review September 3, 2024 08:31
@hvitved hvitved requested review from a team as code owners September 3, 2024 08:31
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.

CPP, Swift changes LGTM. DCA looks fine as well, though performance-wise I can only detect wobble.

We've lost some duplicate nodes in Swift, which is probably a good thing assuming it was expected.

@aschackmull
Copy link
Contributor

Two comments so far:

  • I don't think it's necessary to cut down the size of NodeExImpl based on the configuration - simply restricting the read step relation instead ought to achieve the same and we avoid both scanning the NodeExImpl relation to create the reduced type and the risk of the compiler inserting hard-to-eliminate type-checks.
  • There's a bug in relation to provenance: When we merge the last two steps before a sink where the final step is an implicit read, and both steps have non-empty provenance, then we lose the sink-provenance.

@hvitved hvitved marked this pull request as draft September 18, 2024 09:05
@hvitved hvitved force-pushed the dataflow/node-ex-cached branch 4 times, most recently from 41e416f to dfd35b2 Compare September 19, 2024 12:35
@hvitved hvitved marked this pull request as ready for review September 19, 2024 18:00
@hvitved
Copy link
Contributor Author

hvitved commented Sep 19, 2024

@aschackmull : PR ready for review again.

@hvitved hvitved requested a review from a team as a code owner September 24, 2024 10:14
@github-actions github-actions bot added the Go label Sep 24, 2024
Comment on lines +3327 to +3328
or
n2 = n1.getAnImplicitReadSuccessorAtSink(_)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a relevant addition? Isn't this just for identifying subpath-wrappers through hidden callables?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

That anchored link doesn't work for me. But it makes sense 👍

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.

One minor comment, otherwise LGTM now.

@hvitved hvitved added the depends on internal PR This PR should only be merged in sync with an internal Semmle PR label Sep 24, 2024
@hvitved hvitved merged commit 5b45d36 into github:main Sep 24, 2024
52 of 53 checks passed
@hvitved hvitved deleted the dataflow/node-ex-cached branch September 24, 2024 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C++ DataFlow Library depends on internal PR This PR should only be merged in sync with an internal Semmle PR Go Java no-change-note-required This PR does not need a change note Ruby Swift
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants