Consign the YAML Build Record to the Legacy Incremental Build Path #1243
Conversation
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
The build record and the module dependency graph are currently emitted by "cross-module incremental builds" alongside a "swiftdeps" YAML file that summarizes the compilation session that occurred before this one. This has a number of downsides - We have a hard dependency on a YAML library for just this one thing - With two input files comes a quad-state of cases to handle (if priors desyncs from build record and vice versa... bad things happen) - Moreover, we have observed that mod times returned to us on ext4 on older Ubuntus have a pretty high (probably << 1/100) chance of telling us that the priors file was somehow emitted _before_ the compilation session ended. There's no reason to emit two files when we can just emit one. To start, bump the serialized dependency graph format and absorb the information from the build record into it. Then, teach all the things that instantiate a ModuleDependencyGraph to also hand it a build record. Where they get that build record from is their business, but we can also make our own now.
These tests were stomping over each other and reading stale priors data that was just hanging around from the last run in the driver-computed temporary directory. Even within a single test, this is wrong because multiple modules will read the same priors file. Use the name of the module to diversify things here.
- Argument hash has no business being optional - Introduce BuildRecord.Error to describe serialization failures to clients instead of using a closure - Steal some utilities from InputInfo that don't need to be public
Steal a number of its file reading, writing, and validation routines away from it. These are better managed by the incremental build state.
Time Traveling priors occur when a file system cache lies to us about the state of the incremental build. Since I'd rather not deal with three operating system's worth of file systems to fix that particular problem, let's make sure this can never happen again by relegating one of the files to the legacy incremental build path. The idea is that the "incremental imports" flag controls things here: Incremental Imports Enabled: Read and write Module.priors bitstream files Incremental Imports Disabled: Read and write Module.swiftdeps YAML files The only notable thing needed to make this work is the old fallback that used to "disable" incremental compilation by bailing out of the entire incremental build now instead constructs an empty module dependency graph and fires off a "clean" incremental build. This should be a pretty significant optimization as it now means that on the happy path we can reach a steady state in the incremental build in one build instead of two.
|
@harlanhaskins Has suggested we might be able to drop the Yams dependency even sooner - since YAML is a strict superset of JSON - by making |
|
@swift-ci test |
CodaFi
added a commit
to CodaFi/swift
that referenced
this pull request
Jan 4, 2023
After swiftlang/swift-driver#1243, the new driver no longer emits a dedicated build record when incremental imports are enabled. These tests are directly checking the contents of the YAML build record, and so they have to be restricted to testing modes where we use the legacy driver. rdar://103866776
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.
This PR fixes a pretty harrowing set of Linux bugs we've observed on and off over the years with the incremental build.
The build record and the module dependency graph are currently emitted by "cross-module incremental builds" alongside a "swiftdeps" YAML file that summarizes the compilation session that occurred before this one. This has a number of downsides
There's no reason to emit two files when we can just emit one. To start, bump the serialized dependency graph format and absorb the information from the build record into it. Then, teach all the things that instantiate a ModuleDependencyGraph to also hand it a build record. Where they get that build record from is their business, but we can also make our own now.
Time Traveling priors occur when a file system cache lies to us about the state of the incremental build. Since I'd rather not deal with three operating system's worth of file systems abstractions to fix that particular problem, let's make sure this can never happen again by relegating one of the priors files to the legacy incremental build path.
The idea is that the "incremental imports" flag controls things here:
Incremental Imports Enabled: Read and write Module.priors bitstream files
Incremental Imports Disabled: Read and write Module.swiftdeps YAML files
The only notable thing needed to make this work is the old fallback that used to "disable" incremental compilation by bailing out of the entire incremental build now instead constructs an empty module dependency graph and fires off a "clean" incremental build. This should be a pretty significant optimization as it now means that on the happy path we can reach a steady state in the incremental build in one build instead of two.
I've also cleaned up and fixed a number of incremental build tests that happened to be passing in configurations where it was somehow acceptable to read stale, or in the worst case incorrect, priors files paired with the correct build record files. Finally, I've deleted tests that had anything to do with the time traveling priors phenomenon.