Remove `librustc_ast` session globals #74932

nnethercote · 2020-07-30T03:05:30Z

By moving the data onto Session.

r? @petrochenkov

nnethercote · 2020-07-30T05:27:55Z

@bors try @rust-timer queue

rust-timer · 2020-07-30T05:27:56Z

Awaiting bors try build completion

bors · 2020-07-30T05:28:09Z

⌛ Trying commit 6b3064f7315c9619debcf87aa76cdb1eeb78f237 with merge 3989933cd82bfcf4dd92536dcd76dc61254f4a8d...

bors · 2020-07-30T06:15:38Z

☀️ Try build successful - checks-actions, checks-azure
Build commit: 3989933cd82bfcf4dd92536dcd76dc61254f4a8d (3989933cd82bfcf4dd92536dcd76dc61254f4a8d)

rust-timer · 2020-07-30T06:15:40Z

Queued 3989933cd82bfcf4dd92536dcd76dc61254f4a8d with parent 6e50a22, future comparison URL.

rust-timer · 2020-07-30T12:27:29Z

Finished benchmarking try commit (3989933cd82bfcf4dd92536dcd76dc61254f4a8d): 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

bjorn3 · 2020-07-30T12:33:27Z

This is a slight (up to 0.4%) slowdown.

src/librustc_metadata/creader.rs

src/librustc_hir/Cargo.toml

petrochenkov · 2020-07-30T18:00:21Z

I suspect that majority of these check_names don't actually need to mark the attributes as used.
So were not only doing extra work, but now also need to pass Session to locations where it shouldn't otherwise be.
I need to check a couple of things.

petrochenkov · 2020-07-30T18:47:49Z

I need to check a couple of things.

Blocked on #74952.

nnethercote · 2020-07-30T22:25:18Z

I suspect that majority of these check_names don't actually need to mark the attributes as used.

I was wondering about that, but I had no sense of which were necessary and which weren't, so I kept them all except for the couple that I couldn't keep. It'd be great if there was a clear rule about when check_name is needed.

petrochenkov · 2020-08-01T08:04:45Z

I was wondering about that, but I had no sense of which were necessary and which weren't, so I kept them all except for the couple that I couldn't keep. It'd be great if there was a clear rule about when check_name is needed.

I started working on this and will make a PR tomorrow, most likely.
For most attributes there's only a single place where the attribute is lowered into some semantic form, that's also the place where it needs to be marked as used, so the number of mark_useds is roughly equal to the number of different attributes.

UPD: #74952 is the preliminary implementation.

petrochenkov · 2020-08-02T10:51:35Z

#74952 ended up requiring too much manual work and included some nontrivial cases, so I submitted a simpler #75043 instead.

@nnethercote

rustc_ast: `(Nested)MetaItem::check_name` -> `has_name` For consistency with `Attribute::has_name` which doesn't mark the attribute as used either. Replace all uses of `check_name` with `has_name` outside of rustc, only rustc needs to mark attributes as used. cc rust-lang#74932 r? @nnethercote

@nnethercote

rustc_ast: `(Nested)MetaItem::check_name` -> `has_name` For consistency with `Attribute::has_name` which doesn't mark the attribute as used either. Replace all uses of `check_name` with `has_name` outside of rustc, only rustc needs to mark attributes as used. cc rust-lang#74932 r? @nnethercote

petrochenkov · 2020-08-04T20:49:37Z

#75043 has landed.

nnethercote · 2020-08-05T00:26:39Z

I have updated. Most of the changes outside of rustc are now no longer necessary, but the changes within rustc are still there. So it's still a big change.

nnethercote · 2020-08-08T00:08:08Z

It's a weird rustdoc failure I haven't seen before, only on one of the linux platforms:

error[E0275]: overflow evaluating the requirement `alloc::raw_vec::RawVec<(rustc_ast::ast::UseTree, rustc_ast::ast::NodeId)>: std::marker::Sync`
  |
  = help: consider adding a `#![recursion_limit="256"]` attribute to your crate (`rustc_plugin_impl`)
  = note: required because it appears within the type `std::vec::Vec<(rustc_ast::ast::UseTree, rustc_ast::ast::NodeId)>`
  = note: required because it appears within the type `rustc_ast::ast::UseTreeKind`
  ... <lots more lines like that> ...

This is while running rustdoc on the rustc_plugin_impl module. That seems unrelated to my changes, unless we happened to be really close to the recursion limit previously and my patch changed enough things to tip it over the limit?

I'm not sure what to do here, whether it's spurious and likely to disappear if we try again, or if it's likely to recur. If it's the latter, I guess I should add the recusion_limit="256" like the message suggests?

@jyn514, any ideas?

jyn514 · 2020-08-08T00:40:14Z

I wouldn't expect it to be random. I'm not sure how it's related to your change, but it doesn't seem to be an infinite loop so I would try raising the type limit like it suggests.

These arguments are never `None`.

By moving `{known,used}_attrs` from `SessionGlobals` to `Session`. This means they are accessed via the `Session`, rather than via TLS. A few `Attr` methods and `librustc_ast` functions are now methods of `Session`. All of this required passing a `Session` to lots of functions that didn't already have one. Some of these functions also had arguments removed, because those arguments could be accessed directly via the `Session` argument. `contains_feature_attr()` was dead, and is removed. Some functions were moved from `librustc_ast` elsewhere because they now need to access `Session`, which isn't available in that crate. - `entry_point_type()` --> `librustc_builtin_macros` - `global_allocator_spans()` --> `librustc_metadata` - `is_proc_macro_attr()` --> `Session`

To avoid rustdoc problems.

nnethercote · 2020-08-08T04:39:13Z

I added the 256 recursion limit.

@bors r=petrochenkov

bors · 2020-08-08T04:39:14Z

📌 Commit 96dd044 has been approved by petrochenkov

bors · 2020-08-08T05:59:03Z

⌛ Testing commit 96dd044 with merge e61621c...

bors · 2020-08-08T07:45:56Z

☀️ Test successful - checks-actions, checks-azure
Approved by: petrochenkov
Pushing e61621c to master...

rust-highfive · 2020-08-08T07:46:53Z

📣 Toolstate changed by #74932!

Tested on commit e61621c.
Direct link to PR: #74932

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

@oli-obk

Tested on commit rust-lang/rust@e61621c. Direct link to PR: <rust-lang/rust#74932> 💔 miri on windows: test-fail → build-fail (cc @oli-obk @eddyb @RalfJung). 💔 miri on linux: test-fail → build-fail (cc @oli-obk @eddyb @RalfJung).

@eggyal

do not call black_box on Miri Helps with rust-lang#75274 (but rust-lang#74932 introduced unrelated breakage that will need a separate fix) Cc @eggyal r? @Mark-Simulacrum

rustup Adjust for rust-lang/rust#74932. Blocked on rust-lang/rust#75282.

rust-highfive assigned petrochenkov Jul 30, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 30, 2020

nnethercote force-pushed the rm-ast-session-globals branch 2 times, most recently from e2cb235 to 6b3064f Compare July 30, 2020 04:18

petrochenkov reviewed Jul 30, 2020

View reviewed changes

src/librustc_metadata/creader.rs Outdated Show resolved Hide resolved

src/librustc_hir/Cargo.toml Outdated Show resolved Hide resolved

petrochenkov added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 30, 2020

petrochenkov mentioned this pull request Aug 2, 2020

rustc_ast: (Nested)MetaItem::check_name -> has_name #75043

Merged

Aaron1011 mentioned this pull request Aug 3, 2020

Use TLS less in rustc_span #73110

Closed

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Aug 4, 2020

nnethercote force-pushed the rm-ast-session-globals branch from 6b3064f to 6d83ba1 Compare August 5, 2020 00:22

petrochenkov removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 5, 2020

petrochenkov added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 7, 2020

nnethercote added 4 commits August 8, 2020 12:02

Remove some unnecessary uses of Option.

d6fa011

These arguments are never `None`.

Eliminate librustc_hir's dependency on librustc_session.

3dc8a36

Increase recursion_limit in librustc_plugin_impl.

96dd044

To avoid rustdoc problems.

nnethercote force-pushed the rm-ast-session-globals branch from ccab50d to 96dd044 Compare August 8, 2020 04:38

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 Aug 8, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 8, 2020

bors merged commit e61621c into rust-lang:master Aug 8, 2020

This was referenced Aug 8, 2020

Remove Spans from HIR -- 1/N -- Span collection #72878

Closed

Doc alias checks: ensure only items appearing in search index can use it #74098

Merged

Support #[track_caller] on closures. #74492

Closed

This was referenced Aug 8, 2020

do not call black_box on Miri #75282

Merged

rustup rust-lang/miri#1500

Merged

bors added a commit to rust-lang/miri that referenced this pull request Aug 8, 2020

Auto merge of #1500 - RalfJung:rustup, r=RalfJung

193458d

rustup Adjust for rust-lang/rust#74932. Blocked on rust-lang/rust#75282.

bors added a commit to rust-lang/miri that referenced this pull request Aug 8, 2020

Auto merge of #1500 - RalfJung:rustup, r=RalfJung

302ccf4

rustup Adjust for rust-lang/rust#74932. Blocked on rust-lang/rust#75282.

nnethercote deleted the rm-ast-session-globals branch August 9, 2020 22:03

Nemo157 added a commit to Nemo157/rust that referenced this pull request Aug 30, 2020

Update for moved function from rust-lang#74932

fc8a3ad

cuviper added this to the 1.47.0 milestone May 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `librustc_ast` session globals #74932

Remove `librustc_ast` session globals #74932

nnethercote commented Jul 30, 2020

nnethercote commented Jul 30, 2020

rust-timer commented Jul 30, 2020

bors commented Jul 30, 2020

bors commented Jul 30, 2020

rust-timer commented Jul 30, 2020

rust-timer commented Jul 30, 2020

bjorn3 commented Jul 30, 2020

petrochenkov commented Jul 30, 2020

petrochenkov commented Jul 30, 2020

nnethercote commented Jul 30, 2020

petrochenkov commented Aug 1, 2020 •

edited

Loading

petrochenkov commented Aug 2, 2020

petrochenkov commented Aug 4, 2020

nnethercote commented Aug 5, 2020

nnethercote commented Aug 8, 2020

jyn514 commented Aug 8, 2020

nnethercote commented Aug 8, 2020

bors commented Aug 8, 2020

bors commented Aug 8, 2020

bors commented Aug 8, 2020

rust-highfive commented Aug 8, 2020

Remove librustc_ast session globals #74932

Remove librustc_ast session globals #74932

Conversation

nnethercote commented Jul 30, 2020

nnethercote commented Jul 30, 2020

rust-timer commented Jul 30, 2020

bors commented Jul 30, 2020

bors commented Jul 30, 2020

rust-timer commented Jul 30, 2020

rust-timer commented Jul 30, 2020

bjorn3 commented Jul 30, 2020

petrochenkov commented Jul 30, 2020

petrochenkov commented Jul 30, 2020

nnethercote commented Jul 30, 2020

petrochenkov commented Aug 1, 2020 • edited Loading

petrochenkov commented Aug 2, 2020

petrochenkov commented Aug 4, 2020

nnethercote commented Aug 5, 2020

nnethercote commented Aug 8, 2020

jyn514 commented Aug 8, 2020

nnethercote commented Aug 8, 2020

bors commented Aug 8, 2020

bors commented Aug 8, 2020

bors commented Aug 8, 2020

rust-highfive commented Aug 8, 2020

Remove `librustc_ast` session globals #74932

Remove `librustc_ast` session globals #74932

petrochenkov commented Aug 1, 2020 •

edited

Loading