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 7 pull requests #102915

Merged
merged 15 commits into from
Oct 11, 2022
Merged

Rollup of 7 pull requests #102915

merged 15 commits into from
Oct 11, 2022

Conversation

JohnTitor
Copy link
Member

Successful merges:

Failed merges:

r? @ghost
@rustbot modify labels: rollup

Create a similar rollup

cjgillot and others added 15 commits September 25, 2022 11:36
Before the documentation sometimes referred to an "rwlock" and sometimes to "`RwLock`".
When this was added, the sidebar had a bit more complex style. It can be
removed, now.
Remove unused variable in float formatting.
Consistently write `RwLock`

Before the documentation sometimes referred to an "rwlock" and sometimes to "`RwLock`".
Never panic in `thread::park` and `thread::park_timeout`

fixes rust-lang#102398

`@rustbot` label +T-libs +T-libs-api
…r=m-ou-se

scoped threads: pass closure through MaybeUninit to avoid invalid dangling references

The `main` function defined here looks roughly like this, if it were written as a more explicit stand-alone function:
```rust
// Not showing all the `'lifetime` tracking, the point is that
// this closure might live shorter than `thread`.
fn thread(control: ..., closure: impl FnOnce() + 'lifetime) {
    closure();
    control.signal_done();
    // A lot of time can pass here.
}
```
Note that `thread` continues to run even after `signal_done`! Now consider what happens if the `closure` captures a reference of lifetime `'lifetime`:
- The type of `closure` is a struct (the implicit unnameable closure type) with a `&'lifetime mut T` field. References passed to a function are marked with `dereferenceable`, which is LLVM speak for *this reference will remain live for the entire duration of this function*.
- The closure runs, `signal_done` runs. Then -- potentially -- this thread gets scheduled away and the main thread runs, seeing the signal and returning to the user. Now `'lifetime` ends and the memory the reference points to might be deallocated.
- Now we have UB! The reference that as passed to `thread` with the promise of remaining live for the entire duration of the function, actually got deallocated while the function still runs. Oops.

Long-term I think we should be able to use `ManuallyDrop` to fix this without `unsafe`, or maybe a new `MaybeDangling` type. I am working on an RFC for that. But in the mean time it'd be nice to fix this so that Miri with `-Zmiri-retag-fields` (which is needed for "full enforcement" of all the LLVM flags we generate) stops erroring on scoped threads.

Fixes rust-lang#101983
r? `@m-ou-se`
Move lifetime resolution module to rustc_hir_analysis.

Now that lifetime resolution has been removed from it, this file has nothing to do in `rustc_resolve`.  It's purpose is to compute Debruijn indices for lifetimes, so let's put it in type collection.
… r=GuillaumeGomez

rustdoc: remove unneeded `<div>` wrapper from sidebar DOM

When this was added, the sidebar had a bit more complex style. It can be removed, now.

Preview: https://notriddle.com/notriddle-rustdoc-demos/sidebar-block/std/index.html
@rustbot rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. rollup A PR which is a rollup labels Oct 11, 2022
@JohnTitor
Copy link
Member Author

@bors r+ p=7 rollup=never

@bors
Copy link
Contributor

bors commented Oct 11, 2022

📌 Commit 3011538 has been approved by JohnTitor

It is now in the queue for this repository.

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Oct 11, 2022
@bors
Copy link
Contributor

bors commented Oct 11, 2022

⌛ Testing commit 3011538 with merge bb93450...

@bors
Copy link
Contributor

bors commented Oct 11, 2022

☀️ Test successful - checks-actions
Approved by: JohnTitor
Pushing bb93450 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 11, 2022
@bors bors merged commit bb93450 into rust-lang:master Oct 11, 2022
@rustbot rustbot added this to the 1.66.0 milestone Oct 11, 2022
@rust-timer
Copy link
Collaborator

📌 Perf builds for each rolled up PR:

PR# Perf Build Sha
#102898 50cc1effa82606b0c20156f79f44c8f0066aae4f
#102859 f574c8fbc90567e0e8a24441750c84a7ce98c81f
#102625 a5835852ad18505998ad68bd7768398de8fa97ac
#102589 0dec73f07a01a449460297f19ace3690a50f72dc
#102412 2d19dcd7a2ae2c3656f46b3b1d238af9e64823e7
#102277 35d8cd8146791cc0702f04f3558a8fb3914609bd
#102258 cd9c1ff415915c102328122450b5ef0c12cb84ac

previous master: 1e926f0652

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

@JohnTitor JohnTitor deleted the rollup-5ht99y1 branch October 11, 2022 14:14
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (bb93450): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)
0.6% [0.2%, 1.1%] 12
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.7% [-0.9%, -0.6%] 6
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.2% [-0.9%, 1.1%] 18

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.

mean1 range count2
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
4.2% [3.6%, 4.9%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

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.

mean1 range count2
Regressions ❌
(primary)
2.2% [1.6%, 3.5%] 5
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.2% [1.6%, 3.5%] 5

Footnotes

  1. the arithmetic mean of the percent change 2 3

  2. number of relevant changes 2 3

@rustbot rustbot added the perf-regression Performance regression. label Oct 11, 2022
@Mark-Simulacrum
Copy link
Member

@rust-timer build f574c8fbc90567e0e8a24441750c84a7ce98c81f

@rust-timer
Copy link
Collaborator

Queued f574c8fbc90567e0e8a24441750c84a7ce98c81f with parent 1e926f0, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (f574c8fbc90567e0e8a24441750c84a7ce98c81f): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
1.1% [1.1%, 1.1%] 1
Improvements ✅
(primary)
-0.7% [-0.9%, -0.6%] 6
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.7% [-0.9%, -0.6%] 6

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.

mean1 range count2
Regressions ❌
(primary)
5.5% [2.3%, 7.4%] 5
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-7.4% [-16.5%, -2.3%] 27
Improvements ✅
(secondary)
-2.5% [-4.2%, -0.7%] 59
All ❌✅ (primary) -5.4% [-16.5%, 7.4%] 32

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.

mean1 range count2
Regressions ❌
(primary)
2.2% [1.5%, 4.9%] 17
Regressions ❌
(secondary)
2.4% [2.1%, 2.9%] 14
Improvements ✅
(primary)
-15.1% [-29.0%, -1.5%] 42
Improvements ✅
(secondary)
-7.9% [-10.8%, -2.3%] 13
All ❌✅ (primary) -10.1% [-29.0%, 4.9%] 59

Footnotes

  1. the arithmetic mean of the percent change 2 3

  2. number of relevant changes 2 3

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 18, 2022
@rylev
Copy link
Member

rylev commented Oct 19, 2022

Hmm.... the other PRs in this roll up are either rustdoc changes, library changes (to threading libs), or docs changes so not sure which to test next. Even more curious is that running callgrind-diff for two test cases, the biggest diff in calls is in <rustc_expand::mbe::macro_parser::TtParser>::parse_tt which doesn't make much sense.

@nnethercote
Copy link
Contributor

parse_tt instruction counts are sometimes bimodal (usually, but not always, on tt-muncher) so it might not be worth reading much into that.

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. perf-regression Performance regression. 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. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.