Estimate file versions safely#2753
Merged
Merged
Conversation
For a long time, defineEarlyCutoff has been accessing the Values store directly to compute GetModificationTime values instead of calling use, breaking the invariant. The values are used to associate the rule result to a file version, which gets recorded in the Value as well as used as the key in the Diagnostics store. The problem here is that the GetModificationTime rule computes a new version and mutates the Values store, so if defineEarlyCutoff peeks in the store before GetModificationTime has run, it will grab the old version. This leads to lost diagnostics and potentially to misversioned Values Fixing the problem is tricky, because we cannot simply use GetModificationTime inside defineEarlyCutoff for all rules. There are three issues: 1. Creating a dependency on GetModificationTime. If everything depends on it, then we lose the ability to do early cutoff 2. Creating cycles in the build graph, since GetModificationTime has dependencies itself. Because hls-graph doesn't implement cycle detection (Shake did), it is a nightmare to debug these cycles. 3. Creating overhead, since GetModification time calls the file system for non FOIs and in the past this was very expensive for projects with large cartesian product of module paths and source folders To work around these I had to introduce a new hls-graph primitive, applyWithoutDependency, as well as do a bunch of fragile type tests on the key type to decide on whether to use GetModificationTime or peek into the values store. The type casts could be cleaned up by introducing a type class, but I'm not sure the end result would be any better. To understand the issue and debug the implementation of the fix, I added a number of opentelemety traces which I'm leaving in place in case they could be useful in the future.
wz1000
reviewed
Mar 5, 2022
pepeiborra
marked this pull request as ready for review
March 6, 2022 07:02
wz1000
approved these changes
Mar 6, 2022
Collaborator
|
It would be nice if the excellent description of the problem and the solution was recorded somewhere more durable! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
For a long time,
defineEarlyCutoffhas been accessing theValuesstore directlyto compute
GetModificationTimevalues instead of callinguse, breaking theinvariant. The values are used to associate the rule result to a file version,
which gets recorded in the
Valueas well as used as the key in the Diagnosticsstore.
The problem here is that the
GetModificationTimerule computes a new version andmutates the Values store, so if
defineEarlyCutoffpeeks in the store beforeGetModificationTimehas run, it will grab the old version. This leads to lostdiagnostics and potentially to misversioned
Values.Fixing the problem is tricky, because we cannot simply use
GetModificationTimeinside
defineEarlyCutofffor all rules. There are three issues with this:GetModificationTime: if everything depends on it,then we lose the ability to do early cutoff
GetModificationTimehasdependencies itself. Because hls-graph doesn't implement cycle detection (Shake
did), it is a nightmare to debug these cycles.
GetModificationTimecalls the file system for nonFOIs and in the past this was very expensive for projects with large cartesian
product of module paths and source folders
To work around these I had to introduce a new hls-graph primitive,
applyWithoutDependency, as well as add a bunch more fragile type tests on the keytype to decide on whether to use
GetModificationTimeor peek into the valuesstore. The type casts could be cleaned up by introducing a type class, but I'm
not sure the end result would be any better.
To understand the issue and debug the implementation of the fix, I added a
number of opentelemety traces which I'm leaving in place in case they could be
useful in the future.