Skip to content

Java: Cleanup imports of ExternalFlow #11516

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
merged 5 commits into from
Dec 6, 2022

Conversation

michaelnebel
Copy link
Contributor

@michaelnebel michaelnebel commented Dec 1, 2022

In this PR we cleanup the bi-directional framework imports that were introduced for Models as Data.
The bi-directional imports created a huge SCC of dependencies and removing the importa revealed some missing imports in other parts of the code that were relying on the framework SCC in ExternalFlow.qll.
The code has been modified accordingly and bi-directional imports because of other abstract classes have been introduced where needed.

@michaelnebel michaelnebel force-pushed the java/externalflowcleanup branch from 480c20f to c0ea053 Compare December 1, 2022 12:39
@michaelnebel michaelnebel force-pushed the java/externalflowcleanup branch from c715e44 to 27f2379 Compare December 2, 2022 11:14
@michaelnebel michaelnebel force-pushed the java/externalflowcleanup branch 2 times, most recently from c3cb945 to fa5a94a Compare December 2, 2022 13:47
@github-actions github-actions bot removed the C# label Dec 2, 2022
@michaelnebel michaelnebel force-pushed the java/externalflowcleanup branch from fa5a94a to 6e486d4 Compare December 5, 2022 08:49
@michaelnebel michaelnebel added the no-change-note-required This PR does not need a change note label Dec 5, 2022
@michaelnebel michaelnebel marked this pull request as ready for review December 5, 2022 10:24
@michaelnebel michaelnebel requested a review from a team as a code owner December 5, 2022 10:24
Copy link
Contributor

@atorralba atorralba left a comment

Choose a reason for hiding this comment

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

Mostly looks good to me, thanks for doing this! I added a couple NITs (that you can completely ignore), and one minor fix.

@atorralba
Copy link
Contributor

Wondering if this is worth a DCA run 🤔 (when it works).

@michaelnebel
Copy link
Contributor Author

Wondering if this is worth a DCA run 🤔 (when it works).

This is definitely worth a DCA run! We need to see if there are any sudden drops in performance (or improvements).
We could risk worse performance, if some bi-directional imports have been overlooked that could cause re-evaluation.
Will not merge before we are able to run DCA.

@michaelnebel
Copy link
Contributor Author

DCA looks good.

  • The overall performance looks acceptable.
  • There appears to be some minor differences in a couple of the query execution times (stages), but an overall reduction in the execution time by all stages. This might be a glitch or maybe a change in the execution order.

Merging now.

@michaelnebel michaelnebel merged commit 8e4190d into github:main Dec 6, 2022
@michaelnebel michaelnebel deleted the java/externalflowcleanup branch December 6, 2022 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Java Kotlin 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