Retire `ItemLikeVisitor` trait #96825

kckeiks · 2022-05-07T20:52:06Z

Issue #95004
cc @cjgillot

rust-highfive · 2022-05-07T20:52:09Z

Some changes occurred in src/tools/clippy.

cc @rust-lang/clippy

rust-highfive · 2022-05-07T20:52:10Z

r? @jackh726

(rust-highfive has picked a reviewer for you, use r? to override)

cjgillot · 2022-05-08T13:19:40Z

@bors try @rust-timer queue

rust-timer · 2022-05-08T13:19:41Z

Awaiting bors try build completion.

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

bors · 2022-05-08T13:19:47Z

⌛ Trying commit 18de522d3d1c898efded66bd7a9682fa8f5914ca with merge f39b93d7c3fd9f8a404d16432db96302ec9ab095...

bors · 2022-05-08T14:47:39Z

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

rust-timer · 2022-05-08T14:47:40Z

Queued f39b93d7c3fd9f8a404d16432db96302ec9ab095 with parent 4c09a33, future comparison URL.

rust-timer · 2022-05-08T16:27:57Z

Finished benchmarking commit (f39b93d7c3fd9f8a404d16432db96302ec9ab095): comparison url.

Summary:

Primary benchmarks: 😿 relevant regressions found
Secondary benchmarks: mixed results

	Regressions 😿 (primary)	Regressions 😿 (secondary)	Improvements 🎉 (primary)	Improvements 🎉 (secondary)	All 😿 🎉 (primary)
count¹	2	9	0	6	2
mean²	0.2%	0.6%	N/A	-0.3%	0.2%
max	0.2%	0.8%	N/A	-0.5%	0.2%

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

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

number of relevant changes ↩
the arithmetic mean of the percent change ↩

compiler/rustc_passes/src/check_const.rs

compiler/rustc_passes/src/dead.rs

compiler/rustc_passes/src/hir_id_validator.rs

cjgillot · 2022-05-08T13:06:28Z

compiler/rustc_passes/src/reachable.rs

    if let hir::ItemKind::Impl(hir::Impl { of_trait: Some(ref trait_ref), ref items, .. }) =
-    item.kind
+        item.kind


I wonder if this one also could be implemented without fetching HIR.

If the snippet here is OK, then I will use the same approach here.

The algorithm here needs the Res from hir::TraitRef but impl_trait_ref returns ty::TraitRef which doesn't have that info. I haven't been able to find to a way to get that data without accessing the HIR.

trait_def_id is tcx.impl_trait_ref(item.def_id).def_id.

Sorry I don't follow. We need the Res not the LocalDefId or is it not necessary to check the variant of Res?

https://github.com/rust-lang/rust/pull/96825/files#diff-5d09ebb3a34c2ff24072d173d202b8d24dc57fb831b430c1c1ba48f0f7942de7R340

I don't think we need to check the variant here. Let's do it in a follow-up PR so we can test it independently.

Sounds good. I will leave this snippet here.

// We need only trait impls here, not inherent impls, and only non-exported ones let of_trait = tcx.impl_trait_ref(id.def_id); if !access_levels.is_reachable(id.def_id) { let local_def_ids = tcx .associated_items(id.def_id) .in_definition_order() .filter_map(|assoc_item| assoc_item.def_id.as_local()); worklist.extend(local_def_ids); if !of_trait.def_id.is_local() { return; } worklist.extend( tcx.provided_trait_methods(of_trait.def_id).map(|assoc| assoc.def_id.expect_local()), ); }

compiler/rustc_privacy/src/lib.rs

compiler/rustc_passes/src/debugger_visualizer.rs

compiler/rustc_middle/src/hir/map/mod.rs

klensy · 2022-05-09T16:45:20Z

compiler/rustc_passes/src/check_const.rs

+                        .unwrap_or(false);
+
+                    if !is_implemented {
+                        to_implement.push(tcx.item_name(trait_item_id).to_string());


No need to allocate strings immediately, this can be done when error actually invoked.

No need to allocate strings immediately, this can be done when error actually invoked.

Sorry I almost missed this. I made the changes.

bors · 2022-05-09T22:59:31Z

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

kckeiks · 2022-05-11T16:59:20Z

@cjgillot tests are panicing after rebasing changes from #96473.

The issue is in reachable.rs where calling codegen_fn_attrs before has_codegen_attrs causes this error.

thread 'rustc' panicked at 'DefId(2:3366 ~ core[f529]::ops::deref::Deref::Target) does not have a "codegen_fn_attrs"', compiler/rustc_metadata/src/rmeta/decoder/cstore_impl.rs:123:1

The code that was causing the error

fn has_custom_linkage<'tcx>(tcx: TyCtxt<'tcx>, def_id: LocalDefId) -> bool {
    // Anything which has custom linkage gets thrown on the worklist no
    // matter where it is in the crate, along with "special std symbols"
    // which are currently akin to allocator symbols.
    let codegen_attrs = tcx.codegen_fn_attrs(def_id);
    tcx.def_kind(def_id).has_codegen_attrs()
    && (codegen_attrs.contains_extern_indicator()
        || codegen_attrs.flags.contains(CodegenFnAttrFlags::RUSTC_STD_INTERNAL_SYMBOL)
        // FIXME(nbdd0121): `#[used]` are marked as reachable here so it's picked up by
        // `linked_symbols` in cg_ssa. They won't be exported in binary or cdylib due to their
        // `SymbolExportLevel::Rust` export level but may end up being exported in dylibs.
        || codegen_attrs.flags.contains(CodegenFnAttrFlags::USED)
        || codegen_attrs.flags.contains(CodegenFnAttrFlags::USED_LINKER))
}

There were no errors when I changed the function as shown below.

fn has_custom_linkage<'tcx>(tcx: TyCtxt<'tcx>, def_id: LocalDefId) -> bool {
    // Anything which has custom linkage gets thrown on the worklist no
    // matter where it is in the crate, along with "special std symbols"
    // which are currently akin to allocator symbols.
    if !tcx.def_kind(def_id).has_codegen_attrs() {
        return false;
    }
    let codegen_attrs = tcx.codegen_fn_attrs(def_id);
    codegen_attrs.contains_extern_indicator()
        || codegen_attrs.flags.contains(CodegenFnAttrFlags::RUSTC_STD_INTERNAL_SYMBOL)
        // FIXME(nbdd0121): `#[used]` are marked as reachable here so it's picked up by
        // `linked_symbols` in cg_ssa. They won't be exported in binary or cdylib due to their
        // `SymbolExportLevel::Rust` export level but may end up being exported in dylibs.
        || codegen_attrs.flags.contains(CodegenFnAttrFlags::USED)
        || codegen_attrs.flags.contains(CodegenFnAttrFlags::USED_LINKER)
}

Is this expected with the changes that were merged?

Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>

cjgillot · 2022-05-16T20:29:31Z

@bors r+

bors · 2022-05-16T20:29:33Z

📌 Commit 48fd666 has been approved by cjgillot

bors · 2022-05-17T01:42:19Z

⌛ Testing commit 48fd666 with merge 4962b6d234092418aeb8b2762b81d5d443a7e857...

bors · 2022-05-17T01:46:20Z

💔 Test failed - checks-actions

rust-log-analyzer · 2022-05-17T01:59:46Z

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

kckeiks · 2022-05-17T02:29:55Z

A job failed! Check out the build log: (web) (plain)

@cjgillot looks like the install-mingw.sh script failed: Could not resolve host: ci-mirrors.rust-lang.org but ci-mirrors.rust-lang.org resolves fine for me.

ping ci-mirrors.rust-lang.org 
PING d3auslzgbf1mh.cloudfront.net (99.84.37.5) 56(84) bytes of data.
64 bytes from server-99-84-37-5.ewr52.r.cloudfront.net (99.84.37.5): icmp_seq=1 ttl=229 time=38.0 ms

Have you seen this issue before?

cjgillot · 2022-05-17T05:52:56Z

@bors retry

bors · 2022-05-17T06:51:48Z

⌛ Testing commit 48fd666 with merge 7355d97...

bors · 2022-05-17T09:39:20Z

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 7355d97 to master...

rust-timer · 2022-05-17T11:25:52Z

Finished benchmarking commit (7355d97): comparison url.

Summary:

Primary benchmarks: no relevant changes found
Secondary benchmarks: mixed results

	Regressions 😿 (primary)	Regressions 😿 (secondary)	Improvements 🎉 (primary)	Improvements 🎉 (secondary)	All 😿 🎉 (primary)
count¹	0	6	0	5	0
mean²	N/A	4.2%	N/A	-0.3%	N/A
max	N/A	4.8%	N/A	-0.4%	N/A

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

number of relevant changes ↩
the arithmetic mean of the percent change ↩

rylev · 2022-05-17T13:41:22Z

@cjgillot @kckeiks looks like a performance run was done before this was merged, but I'm not seeing any justification for the regression. In any case, it looks like the regression reproduced after merge.

Interestingly, this regression seems to be taking place exclusively in the externs stress test. It looks like the native_library_kind query is being hit. Since that query is going to be used a lot in the externs crate, it makes sense it would be the one to show regressions. That crate produces a lot of metadata and the cachegrind diff shows a big regression in calls to rustc_metadata::rmeta::decoder::cstore_impl::provide which calls into native_library_kind.

Is there any reason why the provide function would be called so much more now?

Here's what cachegrind looks like for externs debug full:

--------------------------------------------------------------------------------
Ir         
--------------------------------------------------------------------------------
66,096,725  PROGRAM TOTALS

--------------------------------------------------------------------------------
Ir          file:function
--------------------------------------------------------------------------------
67,525,500  ???:<rustc_metadata::rmeta::decoder::cstore_impl::provide::{closure
 2,034,543  ???:<rustc_ast::token::Token as rustc_serialize::serialize::Encodable<rustc_metadata::rmeta::encoder::EncodeContext>>::encode
-1,878,543  ???:<rustc_span::span_encoding::Span as rustc_serialize::serialize::Encodable<rustc_metadata::rmeta::encoder::EncodeContext>>::encode
   810,000  ???:<rustc_passes::hir_id_validator::HirIdValidator as rustc_hir::intravisit::Visitor>::visit_foreign_item
  -810,000  ???:<rustc_passes::hir_id_validator::OuterVisitor as rustc_hir::itemlikevisit::ItemLikeVisitor>::visit_foreign_item
  -666,330  ???:<rustc_hir::hir::OwnerNodes>::node
  -645,000  ???:<rustc_privacy::PrivateItemsInPublicInterfacesVisitor as rustc_hir::intravisit::Visitor>::visit_ty
   594,000  ???:rustc_hir::intravisit::walk_ty::<rustc_typeck::check::wfcheck::CheckTypeWellFormedVisitor>
   522,363  ???:rustc_passes::dead::live_symbols_and_ignored_derived_traits
   452,579  ???:rustc_privacy::check_private_in_public
  -432,000  ???:<rustc_typeck::check::wfcheck::CheckTypeWellFormedVisitor as rustc_hir::intravisit::Visitor>::visit_ty

cjgillot · 2022-05-17T15:39:01Z

@rylev

The previous perf run had identified a regression in the bitmaps and stm32f4 suites due to increased calls to associated_items query. This regression has been removed by an extra commit, and does not appear on the post-merge perf run.

The post-merge perf run shows a completely different regression in the native_library_kind query. (The closure in provide is the function pointer that defines this query.) This is odd, as the corresponding code has not been directly modified. The perf output for externs shows that native_library_kind is executed the same number of times.

I have no idea what happened. There may be an unexpected interaction with #96885 which was merged in between. @kckeiks do you have an idea what happened?

kckeiks · 2022-05-17T17:17:38Z

@rylev @cjgillot
I haven't been able to find the cause for this regression. It's strange because, as cjgillot pointed out, the number of executions for that query has not changed per the perf results and we haven't made direct changes to provide or code that calls it. I will keep investigating.

…t, r=cjgillot Retire `ItemLikeVisitor` trait Issue rust-lang#95004 cc `@cjgillot`

rust-highfive assigned jackh726 May 7, 2022

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. labels May 7, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 7, 2022

This comment has been minimized.

Sign in to view

kckeiks force-pushed the remove-item-like-visitor-trait branch from 8bf26ff to 18de522 Compare May 7, 2022 21:36

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 8, 2022

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels May 8, 2022

cjgillot reviewed May 8, 2022

View reviewed changes

kckeiks force-pushed the remove-item-like-visitor-trait branch 2 times, most recently from 89daab5 to 76238b6 Compare May 9, 2022 00:40

cjgillot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 9, 2022

klensy reviewed May 9, 2022

View reviewed changes

kckeiks force-pushed the remove-item-like-visitor-trait branch from 0f13888 to f7bc51a Compare May 10, 2022 20:04

cjgillot self-assigned this May 10, 2022

allocate string only when error will be emitted

48fd666

Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 17, 2022

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 May 17, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label May 17, 2022

bors merged commit 7355d97 into rust-lang:master May 17, 2022

rustbot added this to the 1.63.0 milestone May 17, 2022

xFrednet pushed a commit to xFrednet/rust that referenced this pull request May 21, 2022

Auto merge of rust-lang#96825 - kckeiks:remove-item-like-visitor-trai…

7062eb4

…t, r=cjgillot Retire `ItemLikeVisitor` trait Issue rust-lang#95004 cc `@cjgillot`

kckeiks mentioned this pull request Jun 11, 2022

Refactor HIR item-like traversal #95004

Closed

7 tasks

Retire ItemLikeVisitor trait #96825

Retire ItemLikeVisitor trait #96825

Conversation

kckeiks commented May 7, 2022

rust-highfive commented May 7, 2022

rust-highfive commented May 7, 2022

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

cjgillot commented May 8, 2022

rust-timer commented May 8, 2022

bors commented May 8, 2022

bors commented May 8, 2022

rust-timer commented May 8, 2022

rust-timer commented May 8, 2022

Footnotes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kckeiks May 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kckeiks May 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented May 9, 2022

kckeiks commented May 11, 2022

cjgillot commented May 16, 2022

bors commented May 16, 2022

bors commented May 17, 2022

bors commented May 17, 2022

rust-log-analyzer commented May 17, 2022

kckeiks commented May 17, 2022 • edited Loading

cjgillot commented May 17, 2022

bors commented May 17, 2022

bors commented May 17, 2022

rust-timer commented May 17, 2022

Footnotes

rylev commented May 17, 2022

cjgillot commented May 17, 2022

kckeiks commented May 17, 2022 • edited Loading

Retire `ItemLikeVisitor` trait #96825

Retire `ItemLikeVisitor` trait #96825

kckeiks May 12, 2022 •

edited

Loading

kckeiks May 12, 2022 •

edited

Loading

kckeiks commented May 17, 2022 •

edited

Loading

kckeiks commented May 17, 2022 •

edited

Loading