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 17 pull requests #93138

Merged
merged 88 commits into from
Jan 21, 2022
Merged

Rollup of 17 pull requests #93138

merged 88 commits into from
Jan 21, 2022

Conversation

matthiaskrgr
Copy link
Member

Successful merges:

Failed merges:

r? @ghost
@rustbot modify labels: rollup

Create a similar rollup

CraftSpider and others added 30 commits January 13, 2022 12:40
This change adds the basic infrastructure for tracking drop ranges in
generator interior analysis, which allows us to exclude dropped types
from the generator type.

Not yet complete, but many of the async/await and generator tests pass.
The main missing piece is tracking branching control flow (e.g. around
an `if` expression). The patch does include support, however, for
multiple yields in th e same block.

Issue rust-lang#57478
The main change needed to make this work is to do a pessimistic over-
approximation for AssignOps. The existing ScopeTree analysis in
region.rs works by doing both left to right and right to left order and
then choosing the most conservative ordering. This behavior is needed
because AssignOp's evaluation order depends on whether it is a primitive
type or an overloaded operator, which runs as a method call.

This change mimics the same behavior as region.rs in
generator_interior.rs.

Issue rust-lang#57478
This is needed to handle cases like `[a, b.await, c]`. `ExprUseVisitor`
considers `a` to be consumed when it is passed to the array, but the
array is not quite live yet at that point. This means we were missing
the `a` value across the await point. Attributing drops to the parent
expression means we do not consider the value consumed until the
consuming expression has finished.

Issue rust-lang#57478
This adds support for branching and merging control flow and uses this
to correctly handle the case where a value is dropped in one branch of
an if expression but not another.

There are other cases we need to handle, which will come in follow up
patches.

Issue rust-lang#57478
Not currently working. Need to flow drop information.
All tests pass now! The issue was that we weren't handling all edges
correctly, but now they are handled consistently.

This includes code to dump a graphviz file for the CFG we built for drop
tracking.

Also removes old DropRanges tests.
1. Add test case for partial drops
2. Simplify code in `propagate_to_fixpoint` and remove most clones
3. Clean up PostOrderIndex creation
Splits drop_ranges into drop_ranges::record_consumed_borrow,
drop_ranges::cfg_build, and drop_ranges::cfg_propagate. The top level
drop_ranges module has an entry point that does all the coordination of
the other three phases, using code original in generator_interior.
This cleans up the refactoring from the previous patch and cleans things
up a bit. Each module has a clear entry point and everything else is
private.
The refactoring mainly keeps the separation between the modules clearer.
For example, process_deferred_edges function moved to cfg_build.rs since
that is really part of building the CFG, not finding the fixpoint.

Also, we use PostOrderId instead of usize in a lot more places now.
Also rearranges the existing arms to be more logical. For example, Break
and Continue come closer to Loop now.
update comment for `ensure_monomorphic_enough`

r? `@RalfJung`
…on-number, r=Mark-Simulacrum

Add script to prevent point releases with same number as existing ones

This will hopefully prevent what happened today with rust-lang#93110 and rust-lang#93121, where we built point release artifacts without changing version numbers, thus requiring another PR to change the version number.

r? `@Mark-Simulacrum`
…ter, r=pietroalbini

Backport the 1.58.1 release notes to master

r? `@ghost`
@rustbot rustbot added T-compiler Relevant to the compiler 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 Jan 20, 2022
@matthiaskrgr
Copy link
Member Author

@bors r+ rollup=never p=17

@bors
Copy link
Contributor

bors commented Jan 20, 2022

📌 Commit dd16431 has been approved by matthiaskrgr

@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 Jan 20, 2022
@Aaron1011
Copy link
Member

This rollup includes an already merged PR #93038

@bors
Copy link
Contributor

bors commented Jan 21, 2022

⌛ Testing commit dd16431 with merge 523be2e...

@bors
Copy link
Contributor

bors commented Jan 21, 2022

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 21, 2022
@bors bors merged commit 523be2e into rust-lang:master Jan 21, 2022
@rustbot rustbot added this to the 1.60.0 milestone Jan 21, 2022
@rust-highfive
Copy link
Collaborator

📣 Toolstate changed by #93138!

Tested on commit 523be2e.
Direct link to PR: #93138

💔 miri on windows: test-pass → test-fail (cc @oli-obk @eddyb @RalfJung).
💔 miri on linux: test-pass → test-fail (cc @oli-obk @eddyb @RalfJung).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Jan 21, 2022
Tested on commit rust-lang/rust@523be2e.
Direct link to PR: <rust-lang/rust#93138>

💔 miri on windows: test-pass → test-fail (cc @oli-obk @eddyb @RalfJung).
💔 miri on linux: test-pass → test-fail (cc @oli-obk @eddyb @RalfJung).
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (523be2e): comparison url.

Summary: This change led to very large relevant mixed results 🤷 in compiler performance.

  • Very large improvement in instruction counts (up to -89.5% on full builds of deeply-nested-async check)
  • Moderate regression in instruction counts (up to 2.6% on full builds of await-call-tree check)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

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

@rustbot rustbot added the perf-regression Performance regression. label Jan 21, 2022
@matthiaskrgr
Copy link
Member Author

probably #91032

@lqd
Copy link
Member

lqd commented Jan 23, 2022

#93165 soft reverted #91032 which landed in this PR: the +879% perf results on deeply-nested-async there make it clear that it was indeed the source of this rollup's perf changes.

@eholk
Copy link
Contributor

eholk commented Jan 23, 2022

The perf summary is interesting. These are instruction counts for running rustc on the test case, right? Not instruction counts for executing the compiled test program?

If so it seems like for deeply-nested-async, this PR prunes a lot from the generator type and gives later phases less work to do, leading to a performance improvement.

For await-call-tree, it looks like maybe we weren't able to prune as much, so we are seeing increased time due to the extra analysis work we are doing.

@pnkfelix
Copy link
Member

The perf summary is interesting. These are instruction counts for running rustc on the test case, right? Not instruction counts for executing the compiled test program?

This is correct. https://perf.rust-lang.org/ is focused solely on resources used by running rustc itself. It does not evaluate the quality of the output code (apart from the fact that rustc itself is built atop rustc).

Your hypothesis sounds plausible, though it is nonetheless a pretty amazing peformance change. Is it perhaps a sign that the benchmark will need revision/replacement? I suppose that depends on whether you think it still serves as a realistic model even after drop range tracking is enabled...

@eholk
Copy link
Contributor

eholk commented Jan 27, 2022

The deeply-nested-async benchmark is extremely synthetic. It's checking to make sure the generator types don't get too large during deeply nested async blocks by making a nest of async { async { async { ... } } } blocks that bottoms out in a call to an async function that calls a deep nest of recursive async functions. That said, my PR reduced the size of generator types by being more precise about what needs captured, so I'd consider the change to be a legitimate speedup for the benchmark.

If we want to keep something closer to the old behavior, I think we can make it so the nest of functions returns something with a destructor. I don't know whether it makes sense to do this by modifying the existing benchmark (which would invalidate old perf data) or adding a very similar benchmark beside the current one. What do you think?

@matthiaskrgr matthiaskrgr deleted the rollup-m8akifd branch February 13, 2022 00:53
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. T-compiler Relevant to the compiler 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.