Skip to content
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

Rollup of 4 pull requests #123795

Merged
merged 9 commits into from
Apr 11, 2024
Merged

Rollup of 4 pull requests #123795

merged 9 commits into from
Apr 11, 2024

Conversation

matthiaskrgr
Copy link
Member

Successful merges:

r? @ghost
@rustbot modify labels: rollup

Create a similar rollup

compiler-errors and others added 9 commits April 10, 2024 13:39
This change makes `build.bootstrap-cache-path` option to be configurable with
`./configure` script, so it can be used like `./configure --bootstrap-cache-path=demo`.

Signed-off-by: onur-ozkan <[email protected]>
…env, r=oli-obk

Make the computation of `coroutine_captures_by_ref_ty` more sophisticated

Currently, we treat all the by-(mut/)ref borrows of a coroutine-closure as having a "closure env" borrowed lifetime.

When we have the given code:
```rust
let x: &'a i32 = ...;
let c = async || {
    let _x = *x;
};
```

Then when we call:
```rust
c()
// which, because `AsyncFn` takes a `&self`, we insert an autoref:
(&c /* &'env {coroutine-closure} */)()
```

We will return a future whose captures contain `&'env i32` instead of `&'a i32`, which is way more restrictive than necessary. We should be able to drop `c` while the future is alive since it's not actually borrowing any data *originating from within* the closure's captures, but since the capture has that `'env` lifetime, this is not possible.

This wouldn't be true, for example, if the closure captured `i32` instead of `&'a i32`, because the `'env` lifetime is actually *necessary* since the data (`i32`) is owned by the closure.

This PR identifies two criteria where we *need* to take the borrow with the closure env lifetime:
1. If the closure borrows data from inside the closure's captures. This is not true if the parent capture is by-ref, OR if the parent capture is by-move and the child capture begins with a deref projection. This is the example described above.
2. If we're dealing with mutable references, since we cannot reborrow `&'env mut &'a mut i32` into `&'a mut i32`, *only* `&'env mut i32`.

See the documentation on `should_reborrow_from_env_of_parent_coroutine_closure` for more info.

**important:** As disclaimer states on that function, luckily, if this heuristic is not correct, then the program is not unsound, since we still borrowck and validate the choices made from this function -- the only side-effect is that the user may receive unnecessary borrowck errors.

Fixes rust-lang#123241
…, r=compiler-errors

Call lower_const_param instead of duplicating the code

Follow up of rust-lang#123689

r? `@oli-obk`

I had this commit in my old branch that I had forgotten about, `@fmease` pointed about this in rust-lang#123689

I've left the branches that are not `Range` as do nothing as that's what we are currently doing but maybe we want to err or something.
…r=clubby789

correct the handling of `bootstrap-cache-path` option

This change makes `build.bootstrap-cache-path` option to be configurable with `./configure` script, so it can be used like `./configure --bootstrap-cache-path=demo`.
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. rollup A PR which is a rollup labels Apr 11, 2024
@matthiaskrgr
Copy link
Member Author

@bors r+ rollup=never p=4

@bors
Copy link
Contributor

bors commented Apr 11, 2024

📌 Commit 1620f33 has been approved by matthiaskrgr

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 11, 2024
@bors
Copy link
Contributor

bors commented Apr 11, 2024

⌛ Testing commit 1620f33 with merge df7daa8...

@bors
Copy link
Contributor

bors commented Apr 11, 2024

☀️ Test successful - checks-actions
Approved by: matthiaskrgr
Pushing df7daa8 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 11, 2024
@bors bors merged commit df7daa8 into rust-lang:master Apr 11, 2024
12 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Apr 11, 2024
@rust-timer
Copy link
Collaborator

📌 Perf builds for each rolled up PR:

PR# Message Perf Build Sha
#123660 Make the computation of coroutine_captures_by_ref_ty more… 5a9506984eae07ba998e1ae1aa1a09865a7d2b76 (link)
#123738 Call lower_const_param instead of duplicating the code 0f6214da9705e8cab98a0ab4cc8900a331253e2a (link)
#123774 Fix typo MaybeUnit -> MaybeUninit 79dbf318eefb0a278db1d5fa9e8cc482265e600b (link)
#123790 correct the handling of bootstrap-cache-path option 632b60cc626234581f2edcea21184e5d37734689 (link)

previous master: 72fe8a0f00

In the case of a perf regression, run the following command for each PR you suspect might be the cause: @rust-timer build $SHA

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (df7daa8): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-4.1% [-4.8%, -3.3%] 2
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -4.1% [-4.8%, -3.3%] 2

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-5.8% [-8.8%, -3.5%] 6
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 675.554s -> 677.211s (0.25%)
Artifact size: 316.02 MiB -> 315.97 MiB (-0.02%)

@matthiaskrgr matthiaskrgr deleted the rollup-deesd9c branch September 1, 2024 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. rollup A PR which is a rollup S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants