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.

Sign up for GitHub

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

Increase default transitive step to 5 #1398

Closed
wants to merge 1 commit into from
Closed

Increase default transitive step to 5 #1398

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion internal/compiler-interface/src/main/contraband-java/xsbti/compile/IncOptions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
*/
public final class IncOptions implements java.io.Serializable {
public static int defaultTransitiveStep() {
return 3;
return 5;
Copy link
Member

Choose a reason for hiding this comment

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

I'm not 100% sure. I think the original heuristics/assumption is that if you had to hop 3 times (as in one invalidation causing more, and not converging on it after 3 cycles), likely the subproject will not converge, and thus it's actually faster to compile all sources in the subproject than to repeatedly compile 10 files at a time for 5 times etc.

There are certainly cases where increasing the hops might help, but #1396 having 500+ sources in one subproject might be more of an outlier than the norm, and if it failed to converge after 5 cycles, and eventually cause full invalidation anyway, the overall performance will degrade. It might be good to sample some open source projects and show that it would improve the total performance in most projects.

Copy link
Member Author

Choose a reason for hiding this comment

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

True. Maybe we should instead show a log.info() or log.warn() when transitive invalidation is triggered? Say something like "Transitive invalidation is triggered. If this behaviour is unwanted, increase the transitive steps."

But then the log might be too spammy...

Copy link

@OndrejSpanel OndrejSpanel Sep 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

and if it failed to converge after 5 cycles, and eventually cause full invalidation anyway

I am not sure I understand the conditional in the sentence above, but it did not fail after 5 cycles. That said, there is an underlying issue which I am convinced is causing wrong dependencies detected, which may prevent convergence. And for sake of completeness, even in my project now, with the issue not fixed, it is enough to use _.withTransitiveStep(4) to prevent the 500 files being compiled, only 15-50 are compiled in the last step, with the total number of compiled files well below 100.

That said, I observe the total compilation time is reduced significantly on dual-core GitHub runner, where it went from 4 minutes to 1:30 in such situation, but when doing the same on my local system, which is 8-core i7-12700 with M2 SSD, the difference is much lower, 34 s with the setting and 50 without it.

show a log.info() or log.warn() when transitive invalidation is triggered

I would welcome this. I started to investigate the issue because I noticed my GitHub runner times are higher than they used to be, and I noticed that only because of receiving warnings about action minutes spent. If there would be such warning, I would know something is unusual much earlier.

I am also not sure how does #1397 interact with this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure I understand the conditional in the sentence above, but it did not fail after 5 cycles. That said, there is an underlying issue which I am convinced is causing wrong dependencies detected, which may prevent convergence. And for sake of completeness, even in my project now, with the issue not fixed, it is enough to use _.withTransitiveStep(4) to prevent the 500 files being compiled, only 15-50 are compiled in the last step, with the total number of compiled files well below 100.

Yes. These two PRs I made are separate from the underlying issue. Just want to reiterate that I really appreciate your hard work in #1396 (comment) and I will use that reproduction to aid our investigation.

I would welcome this. I started to investigate the issue because I noticed my GitHub runner times are higher than they used to be, and I noticed that only because of receiving warnings about action minutes spent. If there would be such warning, I would know something is unusual much earlier.

Thanks for your feedback! Still need to wait for Eugene's opinion but I believe placing a log in the last cycle is definitely something we can do.

I am also not sure how does #1397 interact with this.

#1397 is an optimization that improve's Zinc's cycle stopping condition. In your issue, I noticed that when your compilation ran out of 3 transitive steps, your compilation was already done, but Zinc was being overly pessimistic and started cycle 4.

I am currently thinking about & testing #1397 as that particular change involves Zinc's core invalidation logic. In the past, changes to that particular logic caused serious regressions such as #911 and I want to make sure that #1397 is safe.

Copy link

Choose a reason for hiding this comment

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

I noticed that when your compilation ran out of 3 transitive steps, your compilation was already done,

Does that mean with #1397 in place, even default _.withTransitiveStep(3) would be fine and there is no need to increase it for my project, even assuming fixing the supposed underlying issue will not reduce number of transitive steps needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

I noticed that when your compilation ran out of 3 transitive steps, your compilation was already done,

Does that mean with #1397 in place, even default _.withTransitiveStep(3) would be fine and there is no need to increase it for my project, even assuming fixing the supposed underlying issue will not reduce number of transitive steps needed?

That's the intention.

}
public static double defaultRecompileAllFraction() {
return 0.5;
Expand Down
2 changes: 1 addition & 1 deletion internal/compiler-interface/src/main/contraband/incremental.contra
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ type IncOptions {
@since("1.4.0")

#x public static int defaultTransitiveStep() {
#x return 3;
#x return 5;
#x }
#x public static double defaultRecompileAllFraction() {
#x return 0.5;
Expand Down