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

Calculate visibilities once in resolve #78077

Merged
merged 1 commit into from
Oct 21, 2020
Merged

Conversation

petrochenkov
Copy link
Contributor

Then use them through a query based on resolver outputs.

Item visibilities were previously calculated in three places - initially in rustc_resolve, then in rustc_privacy during type privacy checkin, and then in rustc_metadata during metadata encoding.
The visibility logic is not entirely trivial, especially for things like constructors or enum variants, and all of it was duplicated.

This PR deduplicates all the visibility calculations, visibilities are determined once during early name resolution and then stored in ResolverOutputs and are later available through tcx as a query tcx.visibility(def_id).
(This query existed previously, but only worked for other crates.)

Some special cases (e.g. visibilities for closure types, which are needed for type privacy checking) are not processed in resolve, but deferred and performed directly in the query instead.

@rust-highfive
Copy link
Collaborator

r? @lcnr

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 18, 2020
@petrochenkov
Copy link
Contributor Author

@bors try

@bors
Copy link
Contributor

bors commented Oct 18, 2020

⌛ Trying commit 127846e4b8f7fe34f753c1f6018e291fd38bf171 with merge 7bc5bf62df28f36352ac7daacddb8017d94d9f8a...

Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So looking through this I didn't see anything which looked out of place, but I am not familiar enough with this code to actually review these changes 🤔

compiler/rustc_privacy/src/lib.rs Outdated Show resolved Hide resolved
@lcnr
Copy link
Contributor

lcnr commented Oct 18, 2020

@rust-highfive rust-highfive assigned davidtwco and unassigned lcnr Oct 18, 2020
@bors
Copy link
Contributor

bors commented Oct 18, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: 7bc5bf62df28f36352ac7daacddb8017d94d9f8a (7bc5bf62df28f36352ac7daacddb8017d94d9f8a)

@petrochenkov
Copy link
Contributor Author

petrochenkov commented Oct 18, 2020

Question: is it possible to make a query eval_always for the current crate and not eval_always for other crates?
(Or even make it use some more complex condition?)

As I understand, the query must be eval_always because it accesses data from ResolverOutputs which is built before query system starts working, but for visibilities from other crates it would be undesirable to decode them from metadata again and again.

Does it have to be split into multiple queries in this case - some eval_always and some not?

UPD: It looks like eval_always only affects the incremental case (multiple compilations), and queries are always memoized during a single compilation.

@petrochenkov
Copy link
Contributor Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Oct 18, 2020

⌛ Trying commit 49e528cdbe56463eaaadaa10e7397f3127bb0203 with merge 0b7c494c748e1465d977c8fa3a39cd7e1944787b...

@jyn514 jyn514 added A-visibility Area: Visibility / privacy T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. C-cleanup Category: PRs that clean code up or issues documenting cleanup. labels Oct 18, 2020
@bors
Copy link
Contributor

bors commented Oct 18, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: 0b7c494c748e1465d977c8fa3a39cd7e1944787b (0b7c494c748e1465d977c8fa3a39cd7e1944787b)

@rust-timer
Copy link
Collaborator

Queued 0b7c494c748e1465d977c8fa3a39cd7e1944787b with parent 834821e, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (0b7c494c748e1465d977c8fa3a39cd7e1944787b): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

Then use them through a query based on resolver outputs
Copy link
Member

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, r=me if you're happy with this.

@petrochenkov
Copy link
Contributor Author

@bors r=davidtwco

@bors
Copy link
Contributor

bors commented Oct 19, 2020

📌 Commit cee5521 has been approved by davidtwco

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 19, 2020
@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 19, 2020
@jonas-schievink jonas-schievink added the relnotes-perf Performance improvements that should be mentioned in the release notes. label Oct 19, 2020
@bors
Copy link
Contributor

bors commented Oct 21, 2020

⌛ Testing commit cee5521 with merge 1eaadeb...

@bors
Copy link
Contributor

bors commented Oct 21, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: davidtwco
Pushing 1eaadeb to master...

@Mark-Simulacrum
Copy link
Member

Final perf results were approximately equivalent to the perf run on this PR; some solid improvement on deeply-nested-async benchmark in particular. Good work!

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Feb 14, 2021
Pkgsrc changes:
 * Adjust patches, convert tabs to spaces so that tests pass.
 * Remove patches which are no longer needed (upstream changed)
 * Minor adjustments for SunOS, e.g. disable stack probes.
 * Adjust cargo checksum patching accordingly.
 * Remove commented-out use of PATCHELF on NetBSD, which doesn't work anyway...

Upstream changes:

Version 1.49.0 (2020-12-31)
============================

Language
-----------------------

- [Unions can now implement `Drop`, and you can now have a field in a union
  with `ManuallyDrop<T>`.][77547]
- [You can now cast uninhabited enums to integers.][76199]
- [You can now bind by reference and by move in patterns.][76119] This
  allows you to selectively borrow individual components of a type. E.g.
  ```rust
  #[derive(Debug)]
  struct Person {
      name: String,
      age: u8,
  }

  let person = Person {
      name: String::from("Alice"),
      age: 20,
  };

  // `name` is moved out of person, but `age` is referenced.
  let Person { name, ref age } = person;
  println!("{} {}", name, age);
  ```

Compiler
-----------------------

- [Added tier 1\* support for `aarch64-unknown-linux-gnu`.][78228]
- [Added tier 2 support for `aarch64-apple-darwin`.][75991]
- [Added tier 2 support for `aarch64-pc-windows-msvc`.][75914]
- [Added tier 3 support for `mipsel-unknown-none`.][78676]
- [Raised the minimum supported LLVM version to LLVM 9.][78848]
- [Output from threads spawned in tests is now captured.][78227]
- [Change os and vendor values to "none" and "unknown" for some targets][78951]

\* Refer to Rust's [platform support page][forge-platform-support] for more
information on Rust's tiered platform support.

Libraries
-----------------------

- [`RangeInclusive` now checks for exhaustion when calling `contains`
  and indexing.][78109]
- [`ToString::to_string` now no longer shrinks the internal buffer
  in the default implementation.][77997]
- [`ops::{Index, IndexMut}` are now implemented for fixed sized
  arrays of any length.][74989]

Stabilized APIs
---------------

- [`slice::select_nth_unstable`]
- [`slice::select_nth_unstable_by`]
- [`slice::select_nth_unstable_by_key`]

The following previously stable methods are now `const`.

- [`Poll::is_ready`]
- [`Poll::is_pending`]

Cargo
-----------------------
- [Building a crate with `cargo-package` should now be independently
  reproducible.][cargo/8864]
- [`cargo-tree` now marks proc-macro crates.][cargo/8765]
- [Added `CARGO_PRIMARY_PACKAGE` build-time environment
  variable.]  [cargo/8758] This variable will be set if the crate
  being built is one the user selected to build, either with `-p`
  or through defaults.
- [You can now use glob patterns when specifying packages &
  targets.][cargo/8752]


Compatibility Notes
-------------------
- [Demoted `i686-unknown-freebsd` from host tier 2 to target tier
  2 support.][78746]
- [Macros that end with a semi-colon are now treated as statements
  even if they expand to nothing.][78376]
- [Rustc will now check for the validity of some built-in attributes
  on enum variants.][77015] Previously such invalid or unused
  attributes could be ignored.
- Leading whitespace is stripped more uniformly in documentation
  comments, which may change behavior. You read [this post about
  the changes][rustdoc-ws-post] for more details.
- [Trait bounds are no longer inferred for associated types.][79904]

Internal Only
-------------
These changes provide no direct user facing benefits, but represent
significant improvements to the internals and overall performance
of rustc and related tools.

- [rustc's internal crates are now compiled using the `initial-exec` Thread
  Local Storage model.][78201]
- [Calculate visibilities once in resolve.][78077]
- [Added `system` to the `llvm-libunwind` bootstrap config option.][77703]
- [Added `--color` for configuring terminal color support to bootstrap.][79004]


[75991]: rust-lang/rust#75991
[78951]: rust-lang/rust#78951
[78848]: rust-lang/rust#78848
[78746]: rust-lang/rust#78746
[78376]: rust-lang/rust#78376
[78228]: rust-lang/rust#78228
[78227]: rust-lang/rust#78227
[78201]: rust-lang/rust#78201
[78109]: rust-lang/rust#78109
[78077]: rust-lang/rust#78077
[77997]: rust-lang/rust#77997
[77703]: rust-lang/rust#77703
[77547]: rust-lang/rust#77547
[77015]: rust-lang/rust#77015
[76199]: rust-lang/rust#76199
[76119]: rust-lang/rust#76119
[75914]: rust-lang/rust#75914
[74989]: rust-lang/rust#74989
[79004]: rust-lang/rust#79004
[78676]: rust-lang/rust#78676
[79904]: rust-lang/rust#79904
[cargo/8864]: rust-lang/cargo#8864
[cargo/8765]: rust-lang/cargo#8765
[cargo/8758]: rust-lang/cargo#8758
[cargo/8752]: rust-lang/cargo#8752
[`slice::select_nth_unstable`]: https://doc.rust-lang.org/nightly/std/primitive.slice.html#method.select_nth_unstable
[`slice::select_nth_unstable_by`]: https://doc.rust-lang.org/nightly/std/primitive.slice.html#method.select_nth_unstable_by
[`slice::select_nth_unstable_by_key`]: https://doc.rust-lang.org/nightly/std/primitive.slice.html#method.select_nth_unstable_by_key
[`hint::spin_loop`]: https://doc.rust-lang.org/stable/std/hint/fn.spin_loop.html
[`Poll::is_ready`]: https://doc.rust-lang.org/stable/std/task/enum.Poll.html#method.is_ready
[`Poll::is_pending`]: https://doc.rust-lang.org/stable/std/task/enum.Poll.html#method.is_pending
[rustdoc-ws-post]: https://blog.guillaume-gomez.fr/articles/2020-11-11+New+doc+comment+handling+in+rustdoc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-visibility Area: Visibility / privacy C-cleanup Category: PRs that clean code up or issues documenting cleanup. merged-by-bors This PR was explicitly merged by bors. relnotes-perf Performance improvements that should be mentioned in the release notes. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants