Rollup of 3 pull requests #147900
Merged
Rollup of 3 pull requests #147900
+453
−409
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
This simplifies the initial conditional, and will allow reusing the variable in subsequent checks.
This suppresses warnings on things like `Result<(), !>`, which helps simplify code using the common pattern of having an `Error` associated type: code will only have to check the error if there is a possibility of error.
This makes it easier to review without cross-referencing each test function with its invocation.
In `setup_dep_graph`, we set up a session directory for the current
incremental compilation session, load the dep graph, and then GC stale
incremental compilation sessions for the crate. The freshly-created
session directory ends up in this list of potentially-GC'd directories
but in practice is not typically even considered for GC because the new
directory is neither finalized nor `is_old_enough_to_be_collected`.
Unfortunately, `is_old_enough_to_be_collected` is a simple time check,
and if `load_dep_graph` is slow enough it's possible for the
freshly-created session directory to be tens of seconds old already.
Then, old enough to be *eligible* to GC, we try to `flock::Lock` it as
proof it is not owned by anyone else, and so is a stale working
directory.
Because we hold the lock in the same process, the behavior of
`flock::Lock` is dependent on platform-specifics about file locking
APIs. `fcntl(F_SETLK)`-style locks used on non-Linux Unices do not
provide mutual exclusion internal to a process. `fcntl_locking(2)` on
Linux describes some relevant problems:
```
The record locks described above are associated with the process
(unlike the open file description locks described below). This
has some unfortunate consequences:
* If a process closes any file descriptor referring to a file,
then all of the process's locks on that file are released, [...]
* The threads in a process share locks. In other words, a
multithreaded program can't use record locking to ensure that
threads don't simultaneously access the same region of a file.
```
`fcntl`-locks will appear to succeed to lock the fresh incremental
compilation directory, at which point we can remove it just before using
it later for incremental compilation. Saving incremental compilation
state later fails and takes rustc with it with an error like
```
[..]/target/debug/incremental/crate-<hash>/<name>/dep-graph.part.bin: No such file or directory (os error 2)
```
The release-lock-on-close behavior has uncomfortable consequences for
the freshly-opened file description for the lock, but I think in
practice isn't an issue. If we would close the file, we failed to
acquire the lock, so someone else had the lock ad we're not releasing
locks prematurely.
`flock(LOCK_EX)` doesn't seem to have these same issues, and because
`flock::Lock::new` always opens a new file description when locking, I
don't think Linux can have this issue.
From reading `LockFileEx` on MSDN I *think* Windows has locking
semantics similar to `flock`, but I haven't tested there at all.
My conclusion is that there is no way to write a pure-POSIX
`flock::Lock::new` which guarantees mutual exclusion across different
file descriptions of the same file in the same process, and
`flock::Lock::new` must not be used for that purpose. So, instead, avoid
considering the current incremental session directory for GC in the
first place. Our own `sess` is evidence we're alive and using it.
…r=lcnr,petrochenkov Deny-by-default never type lints In Rust [1.89.0](https://github.com/rust-lang/rust/milestone/133) we started emitting these lints in dependencies. I discussed the future steps with `@lcnr` and we think that before stabilizing the never type (and doing the breaking changes) we should deny the lints for ~4 releases. This PR marks `never_type_fallback_flowing_into_unsafe` and `dependency_on_unit_never_type_fallback` lints as deny-by-default. Tracking: - rust-lang#35121 Related: - rust-lang#141937
…e-result-unit-uninhabited, r=fmease unused_must_use: Don't warn on `Result<(), Uninhabited>` or `ControlFlow<Uninhabited, ()>` This suppresses warnings on things like `Result<(), !>`, which helps simplify code using the common pattern of having an `Error` associated type: code will only have to check the error if there is a possibility of error. This will, for instance, help with future refactorings of `write!` in the standard library. As this is a user-visible change to lint behavior, it'll require a lang FCP. --- My proposal, here, is that we handle this simple case to make common patterns more usable. This does not rule out the possibility of adding more cases in the future, including general trait-based cases. However, I don't think we should make this common case wait on the more general cases. In particular, this solution does not close any doors on replacing this special case with a general case. This would unblock some planned work in the standard library to make `write!` more usable for infallible cases (e.g. writing into a `Vec` or `String`).
…=nnethercote Do not GC the current active incremental session directory when building a relatively large repo (https://github.com/oxidecomputer/omicron) on illumos under heavy CPU pressure, i saw some rustc invocations die like: ``` [..]/target/debug/incremental/<crate>-<hash>/<name>/dep-graph.part.bin: No such file or directory (os error 2) ``` a bit of debugging later and it seems that if the system is very slow, Unix-flavored `flock::Lock::new()` doesn't quite get the mutual exclusion `garbage_collect_session_directories` expects. before this patch i could reproduce this with the crate `nexus_db_queries` (in that repo) by pinning the full `cargo build` to one core and having a busy loop fighting on that same core. with this patch i cannot reproduce the issue. i took a look at how `flock::Lock` is used and i think this is the only problematic use, so i figure i'll propose this change particularly since i don't think file locking can be made.. good... for Unix in general. ------ In `setup_dep_graph`, we set up a session directory for the current incremental compilation session, load the dep graph, and then GC stale incremental compilation sessions for the crate. The freshly-created session directory ends up in this list of potentially-GC'd directories but in practice is not typically even considered for GC because the new directory is neither finalized nor `is_old_enough_to_be_collected`. Unfortunately, `is_old_enough_to_be_collected` is a simple time check, and if `load_dep_graph` is slow enough it's possible for the freshly-created session directory to be tens of seconds old already. Then, old enough to be *eligible* to GC, we try to `flock::Lock` it as proof it is not owned by anyone else, and so is a stale working directory. Because we hold the lock in the same process, the behavior of `flock::Lock` is dependent on platform-specifics about file locking APIs. `fcntl(F_SETLK)`-style locks used on non-Linux Unices do not provide mutual exclusion internal to a process. `fcntl_locking(2)` on Linux describes some relevant problems: ``` The record locks described above are associated with the process (unlike the open file description locks described below). This has some unfortunate consequences: * If a process closes any file descriptor referring to a file, then all of the process's locks on that file are released, [...] * The threads in a process share locks. In other words, a multithreaded program can't use record locking to ensure that threads don't simultaneously access the same region of a file. ``` `fcntl`-locks will appear to succeed to lock the fresh incremental compilation directory, at which point we can remove it just before using it later for incremental compilation. Saving incremental compilation state later fails and takes rustc with it with an error like ``` [..]/target/debug/incremental/crate-<hash>/<name>/dep-graph.part.bin: No such file or directory (os error 2) ``` The release-lock-on-close behavior has uncomfortable consequences for the freshly-opened file description for the lock, but I think in practice isn't an issue. If we would close the file, we failed to acquire the lock, so someone else had the lock ad we're not releasing locks prematurely. `flock(LOCK_EX)` doesn't seem to have these same issues, and because `flock::Lock::new` always opens a new file description when locking, I don't think Linux can have this issue. From reading `LockFileEx` on MSDN I *think* Windows has locking semantics similar to `flock`, but I haven't tested there at all. My conclusion is that there is no way to write a pure-POSIX `flock::Lock::new` which guarantees mutual exclusion across different file descriptions of the same file in the same process, and `flock::Lock::new` must not be used for that purpose. So, instead, avoid considering the current incremental session directory for GC in the first place. Our own `sess` is evidence we're alive and using it.
rustbot
added
S-waiting-on-review
|
Rollup of everything. @bors r+ rollup=never p=5 |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
📌 Perf builds for each rolled up PR:
previous master: c0c37cafdc In the case of a perf regression, run the following command for each PR you suspect might be the cause: |
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing c0c37ca (parent) -> ebe145e (this PR) Test differencesShow 155 test diffsStage 1
Stage 2
Additionally, 153 doctest diffs were found. These are ignored, as they are noisy. Job group index
Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard ebe145eca703c74525d4a38ed8021d575a23fa30 --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
|
Finished benchmarking commit (ebe145e): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results (secondary -1.9%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary 13.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 471.783s -> 473.415s (0.35%) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
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.
Successful merges:
Result<(), Uninhabited>orControlFlow<Uninhabited, ()>#147382 (unused_must_use: Don't warn onResult<(), Uninhabited>orControlFlow<Uninhabited, ()>)r? @ghost
@rustbot modify labels: rollup
Create a similar rollup