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

Remove many RefCells from DocContext #82305

Merged
merged 1 commit into from
Feb 23, 2021
Merged

Conversation

camelid
Copy link
Member

@camelid camelid commented Feb 19, 2021

I left some of them so this change doesn't balloon in size and because
removing the RefCell in DocContext.resolver would require compiler
changes.

Thanks to @jyn514 for making this a lot easier with #82020!

r? @jyn514

@camelid camelid added C-cleanup Category: PRs that clean code up or issues documenting cleanup. I-compiletime Issue: Problems and improvements with respect to compile times. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Feb 19, 2021
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 19, 2021
@camelid
Copy link
Member Author

camelid commented Feb 19, 2021

Waiting for CI to pass and then will do perf run.

crate resolver: Rc<RefCell<interface::BoxedResolver>>,
/// Used for normalization.
///
/// Most of this logic is copied from rustc_lint::late.
crate param_env: Cell<ParamEnv<'tcx>>,
/// Later on moved into `cache`
crate renderinfo: RefCell<RenderInfo>,
crate renderinfo: RenderInfo,
/// Later on moved through `clean::Crate` into `cache`
crate external_traits: Rc<RefCell<FxHashMap<DefId, clean::Trait>>>,
Copy link
Member Author

Choose a reason for hiding this comment

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

I left this one in because it seems the lifetimes get tricky because it's moved around.

Copy link
Member

Choose a reason for hiding this comment

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

/// Table synthetic type parameter for `impl Trait` in argument position -> bounds
crate impl_trait_bounds: RefCell<FxHashMap<ImplTraitParam, Vec<clean::GenericBound>>>,
crate impl_trait_bounds: FxHashMap<ImplTraitParam, Vec<clean::GenericBound>>,
crate fake_def_ids: RefCell<FxHashMap<CrateNum, DefIndex>>,
Copy link
Member Author

Choose a reason for hiding this comment

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

I left this one in because

  1. It would require semi-significant changes
  2. Ideally we're going to get rid of fake_def_ids at some point anyway (I think Joshua has an idea for that)

Copy link
Member

Choose a reason for hiding this comment

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

Well, "at some point" is probably pretty far off. But I'm fine with waiting for a follow-up PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Note to future readers: I was able to remove this one in #82382.

@@ -25,7 +25,7 @@ impl<'a, 'tcx> LibEmbargoVisitor<'a, 'tcx> {
crate fn new(cx: &'a mut crate::core::DocContext<'tcx>) -> LibEmbargoVisitor<'a, 'tcx> {
LibEmbargoVisitor {
Copy link
Member Author

Choose a reason for hiding this comment

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

Might I say, LibEmbargoVisitor is one of the most interesting names I've seen in rust-lang/rust.

@camelid
Copy link
Member Author

camelid commented Feb 19, 2021

I've been eagerly watching the logs and the rustdoc testsuite just passed!

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 19, 2021
@bors
Copy link
Contributor

bors commented Feb 19, 2021

⌛ Trying commit ab1be029dc3b5d2c2efa94ad7e508172ab5617a0 with merge 42c967a8b9046d57e776497be0724b4e1b1931c7...

@bors
Copy link
Contributor

bors commented Feb 19, 2021

☀️ Try build successful - checks-actions
Build commit: 42c967a8b9046d57e776497be0724b4e1b1931c7 (42c967a8b9046d57e776497be0724b4e1b1931c7)

@rust-timer
Copy link
Collaborator

Queued 42c967a8b9046d57e776497be0724b4e1b1931c7 with parent 9b471a3, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (42c967a8b9046d57e776497be0724b4e1b1931c7): 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
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 20, 2021
@camelid
Copy link
Member Author

camelid commented Feb 20, 2021

Fairly small improvements, but they're in the right direction, and it cleans up the code.

src/librustdoc/clean/inline.rs Outdated Show resolved Hide resolved
@camelid
Copy link
Member Author

camelid commented Feb 20, 2021

Hmm, seems I have conflicts but bors hasn't reported yet.

@camelid camelid force-pushed the no-more-refcell branch 2 times, most recently from af3e95d to fb96100 Compare February 21, 2021 00:44
@camelid
Copy link
Member Author

camelid commented Feb 21, 2021

Rebased.

@camelid
Copy link
Member Author

camelid commented Feb 21, 2021

@bors r=@jyn514

@bors
Copy link
Contributor

bors commented Feb 21, 2021

📌 Commit fb9610030cfd4596d3c1fc5074338d38b5b2661d has been approved by jyn514

@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 Feb 21, 2021
@bors
Copy link
Contributor

bors commented Feb 22, 2021

☔ The latest upstream changes (presumably #82393) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Feb 22, 2021
@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Feb 22, 2021
I left some of them so this change doesn't balloon in size and because
removing the RefCell in `DocContext.resolver` would require compiler
changes.

Thanks to `@jyn514` for making this a lot easier with rust-lang#82020!
@camelid
Copy link
Member Author

camelid commented Feb 22, 2021

@bors r=@jyn514

@bors
Copy link
Contributor

bors commented Feb 22, 2021

📌 Commit e4ac499 has been approved by jyn514

@bors
Copy link
Contributor

bors commented Feb 22, 2021

🌲 The tree is currently closed for pull requests below priority 1000. This pull request will be tested once the tree is reopened.

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 22, 2021
@jyn514
Copy link
Member

jyn514 commented Feb 22, 2021

@bors rollup=iffy

This only affects rustdoc's perf, not the compiler's.

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 23, 2021
Rollup of 10 pull requests

Successful merges:

 - rust-lang#81629 (Point out implicit deref coercions in borrow)
 - rust-lang#82113 (Improve non_fmt_panic lint.)
 - rust-lang#82258 (Implement -Z hir-stats for nested foreign items)
 - rust-lang#82296 (Support `pub` on `macro_rules`)
 - rust-lang#82297 (Consider auto derefs before warning about write only fields)
 - rust-lang#82305 (Remove many RefCells from DocContext)
 - rust-lang#82308 (Lower condition of `if` expression before it's "then" block)
 - rust-lang#82311 (Jsondocck improvements)
 - rust-lang#82362 (Fix mir-cfg dumps)
 - rust-lang#82391 (disable atomic_max/min tests in Miri)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 8e67fe5 into rust-lang:master Feb 23, 2021
@rustbot rustbot added this to the 1.52.0 milestone Feb 23, 2021
@camelid camelid deleted the no-more-refcell branch February 24, 2021 04:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. I-compiletime Issue: Problems and improvements with respect to compile times. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

6 participants