feat: add option to exit early if analysis cache is discarded

[mattem]

Adds --fail_on_analysis_cache_discard option that allows the build to exit early if the analysis cache would have been discarded.

The flag name is of course open to bikeshedding etc.

fixes #16804

[acecilia]

👋 What is the status of this PR?

Context: I have a job that takes average 5min to run. The build was inadvertently discarding the analysis cache. When fixed, the average job time went down to 2min 😮

[gregestren]

👋 What is the status of this PR?

Context: I have a job that takes average 5min to run. The build was inadvertently discarding the analysis cache. When fixed, the average job time went down to 2min 😮

We need to integrate feedback on exactly how the information is communicated. See https://github.com/bazelbuild/bazel/pull/16805#discussion_r1119205478.

I'm not sure where this fits on @mattem 's current workload. I'll see if I can clarify the linked comment a bit more clearly, since it leaves room for interpretation.

[gregestren]

I'm not sure where this fits on @mattem 's current workload. I'll see if I can clarify the linked comment a bit more clearly, since it leaves room for interpretation.

See my just-posted inline comment (https://github.com/bazelbuild/bazel/pull/16805/files#r1136437606).

[mattem]

Sorry, this is stalled on me. I'm juggling quite a bit currently, but I'll see if I can work on this later this week. @gregestren Those comments above make sense to me on a quick glance. Did you want the addition to the finished event to land separately?

[gregestren]

I support the finished approach as the sole solution to this problem. i.e. an iteration of the code I shared with a new type vs. CONFLICTING_CONFIGURATIONS. I think this addresses concerns from all sides.

[gregestren]

Just back from vacation last week. Looking forward to reviewing the updates...

[gregestren]

Since we're getting close to submission, I want to signal boost @mattem 's old comment about flag naming.

The current name is --fail_on_analysis_cache_discard. @mattem noted an alternative suggestion is --disallow_analysis_cache_discards. Anyone else with preferences before we lock on the current naming?

[brentleyjones]

I like --disallow_analysis_cache_discards.

[alexeagle]

If it's a boolean flag maybe --noallow_analysis_cache_discard equiv. to --allow_analysis_cache_discard=false ?

[gregestren]

I'm fine with allow_analysis_cache_discard (the core flag is defined with as a positive action, without negations). And the boolean parsing logic automatically supports the no variant.

I'm generally fine with any naming with my only preference being the "positive action" pattern.

[michaeledgar]

I'd also prefer the flag proposed by alexeagle@, --allow_analysis_cache_discard=<boolean> with standard Boolean magic parsing.

[michaeledgar]

Sorry, I totally misclicked.

[gregestren]

Hey @mattem - I saw you had some new commits recently. Were you ready for the next (last?) round?

[mattem]

Hey @mattem - I saw you had some new commits recently. Were you ready for the next (last?) round?

Yes please! I thought I saw more comments from @michaeledgar, but now I don't see them.

I'll fix up the conflict too in the next push.

[gregestren]

Everything looks good to me, aside from the branch conflict (I imagine it's not a big one) and the failing CI.

One of the CI failures is:

Executing tests from //src/test/java/com/google/devtools/build/lib/analysis:AnalysisTests
WARNING: Aborting evaluation while evaluating ConfiguredTargetKey{label=//extra:extra, config=BuildConfigurationKey[3a3d3f7c9a7871f29abcbb7d967567a8c72c6b24264b23c2240587564728cf0f]}
com.google.devtools.build.lib.skyframe.ConfiguredTargetEvaluationExceptions$ReportedException: com.google.devtools.build.lib.skyframe.ConfiguredValueCreationException: 
	at com.google.devtools.build.lib.skyframe.ConfiguredTargetFunction.compute(ConfiguredTargetFunction.java:276)
	at com.google.devtools.build.skyframe.AbstractParallelEvaluator$Evaluate.run(AbstractParallelEvaluator.java:503)
	at com.google.devtools.build.lib.concurrent.PriorityWorkerPool.dequeueCpuHeavyTaskAndRun(PriorityWorkerPool.java:428)
	at com.google.devtools.build.lib.concurrent.PriorityWorkerPool$WorkerThread.runLoop(PriorityWorkerPool.java:401)
	at com.google.devtools.build.lib.concurrent.PriorityWorkerPool$LoopStarter.run(PriorityWorkerPool.java:370)
	at java.base/java.util.concurrent.ForkJoinTask$RunnableExecuteAction.exec(ForkJoinTask.java:1426)
	at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:290)
	at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1020)
	at java.base/java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1656)
	at java.base/java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1594)
	at java.base/java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:183)
Caused by: com.google.devtools.build.lib.skyframe.ConfiguredValueCreationException: 
...

That looks related. Any idea what's going on?

[gregestren]

Note that the current naming is --disallow_analysis_cache_discards in reference to https://github.com/bazelbuild/bazel/pull/16805#issuecomment-1496233870.

I'm not going to take any further position on flag naming (that vs. the later suggestions) and will approve once the CI passes. It's been a long time in development and I don't want to block it further. But anyone with ongoing preferences please feel free to further discuss with @mattem .

[uri-canva]

I also think --allow_analysis_cache_discard as a boolean is a good idea to avoid double negatives such as --disallow_analysis_cache_discards=false or --nodisallow_analysis_cache_discards.

But I'm also really looking forward to this so I'll take it regardless of what it's called. 😂

[ustamansangat]

I have a similar use case like this where some test script was running bazel queries with a different set of flags causing the analysis cache to be discarded. I noticed this when build times went up.

[gregestren]

I think this is ready to land aside from landing the final flag name and vetting the failed presubmit checks.

If @mattem is busy would anyone else like to carry this through?

[mattem]

Will endeavour to push this over the line by the end of the week!

[mattem]

Didn't quite manage the end of the week, but I believe this has everything addressed, with the flag named allow_analysis_cache_discard, and CI green. @gregestren 🙏

[gregestren]

Awesome! Do you happen to have a recap of the latest diff compared to before this week? Just trying to refresh my memory.

[mattem]

I believe the only real difference is the flag name change, and changed the variable names to follow suite. Only other work I did in the last commit was to resolve merge conflicts.

[brentleyjones]

@bazel-io flag

[iancha1992]

@bazel-io fork 6.4.0

[iancha1992]

@gregestren @mattem When cherry-pick was attempted to release-6.4.0, there were merge conflicts:

1. src/main/java/com/google/devtools/build/lib/analysis/BuildView.java We may need another commit that introduced the lines below:

skyframeBuildView.setConfiguration(
    eventHandler, topLevelConfig, viewOptions.maxConfigChangesToShow);

2. src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java We many need another prior commit that has previously affected the setConfiguration function and changed the function name from setConfigurations to setConfiguration

3. src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java Need a commit that changed the variable name from setConfigurationsForTesting to setConfigurationForTesting

cc: @bazelbuild/triage

[gregestren]

I thought we had a similar challenge with the original merge somewhere? If so, it should be straightforward to resolve.

I'll get to this as soon as I can. What's the deadline for 6.4.0?

[brentleyjones]

What's the deeadline for 6.4.0?

https://github.com/bazelbuild/bazel/issues/19035

Expected first release candidate date: 2023-09-18

[iancha1992]

Hi, @gregestren @brentleyjones. I just wanted to check up on this. Are we still pushing this to be part of 6.4.0? The expected first release candidate date is next Monday (9/18/2023), just FYI.

[gregestren]

Yes. I was away for two weeks, just back today. I'll try to prep up a new patch...