-
-
Notifications
You must be signed in to change notification settings - Fork 352
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
Estimate file versions safely #2753
Merged
Merged
Commits on Mar 5, 2022
-
Configuration menu - View commit details
-
Copy full SHA for aa2209f - Browse repository at this point
Copy the full SHA aa2209fView commit details -
Configuration menu - View commit details
-
Copy full SHA for 1099316 - Browse repository at this point
Copy the full SHA 1099316View commit details -
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.
Configuration menu - View commit details
-
Copy full SHA for d0fbe71 - Browse repository at this point
Copy the full SHA d0fbe71View commit details -
Configuration menu - View commit details
-
Copy full SHA for 421aa1a - Browse repository at this point
Copy the full SHA 421aa1aView commit details -
Configuration menu - View commit details
-
Copy full SHA for 6bb138b - Browse repository at this point
Copy the full SHA 6bb138bView commit details -
Configuration menu - View commit details
-
Copy full SHA for 1490002 - Browse repository at this point
Copy the full SHA 1490002View commit details
Commits on Mar 6, 2022
-
Configuration menu - View commit details
-
Copy full SHA for 477dfb2 - Browse repository at this point
Copy the full SHA 477dfb2View commit details
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.