Refactor HIR item-like traversal (part 1) by kckeiks · Pull Request #95655 · rust-lang/rust

kckeiks · 2022-04-04T20:12:57Z

Issue #95004

Create hir_crate_items query which traverses tcx.hir_crate(()).owners to return a hir::ModuleItems
use tcx.hir_crate_items in tcx.hir().items() to return an iterator of hir::ItemId
use tcx.hir_crate_items to introduce a tcx.hir().par_items(impl Fn(hir::ItemId)) to traverse all items in parallel;

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

cc @cjgillot

rust-highfive · 2022-04-04T20:13:00Z

Some changes occurred in src/tools/clippy.

cc @rust-lang/clippy

rust-highfive · 2022-04-04T20:13:02Z

r? @petrochenkov

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

cjgillot

Thanks @kckeiks. This is a good start. I have a few remarks/questions.

cjgillot · 2022-04-04T20:29:22Z

Could you keep filling submodules? It may prove useful to implement for_each_module.

Yes, I can update this method. I think for_each_module is already there.

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

Line 679 in 6a9080b

pub fn for_each_module(self, f: impl Fn(LocalDefId)) {

Actually, that one starts from the root mod so I may have to do something else here.

I mean: for_each_module currently walks hir_module_items recursively. par_for_each_module does this in recursive form. Having hir_crate_items.submodules as the list of all the modules in the crate will make those two functions simple loops over it.

I see what you mean. I will update for_each_module now that I'm collecting modules.

cjgillot · 2022-04-04T20:30:50Z

Reading the code, I'm not sure that items starts with CRATE_DEF_ID. It should.

Now that you mention it, looking at where the walk begins in walk_toplevel_module, it seems like CRATE_DEF_ID does not get added to items. Am I reading that right?

kckeiks · 2022-04-05T18:55:32Z

The job x86_64-gnu-tools failed! Check out the build log: (web) (plain)
Click to see the possible cause of the failure (guessed by this bot)

Updated clippy test.

cjgillot · 2022-04-05T20:08:09Z

@bors try @rust-timer queue

rust-timer · 2022-04-05T20:08:10Z

Awaiting bors try build completion.

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

bors · 2022-04-05T20:08:18Z

⌛ Trying commit 6ef0f16d6da22d5508d0299f654368ce263ff2f2 with merge 269a8ae2d5d9ee215834795c69b97483f02b20bf...

bors · 2022-04-05T21:38:47Z

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

rust-timer · 2022-04-05T21:38:49Z

Queued 269a8ae2d5d9ee215834795c69b97483f02b20bf with parent f262ca1, future comparison URL.

rust-timer · 2022-04-05T22:52:30Z

Finished benchmarking commit (269a8ae2d5d9ee215834795c69b97483f02b20bf): comparison url.

Summary:

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

	Regressions 😿 (primary)	Regressions 😿 (secondary)	Improvements 🎉 (primary)	Improvements 🎉 (secondary)	All 😿 🎉 (primary)
count¹	14	15	1	0	15
mean²	0.2%	0.3%	-0.4%	N/A	0.2%
max	0.3%	0.3%	-0.4%	N/A	-0.4%

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 led 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 ↩

kckeiks · 2022-04-06T01:23:31Z

@cjgillot I imagine that the performance regression comes from the traversal that the visitor does in hir_crate_items.

cjgillot · 2022-04-10T08:33:18Z

@bors try @rust-timer queue

rust-timer · 2022-04-10T08:33:20Z

Awaiting bors try build completion.

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

bors · 2022-04-10T08:33:27Z

⌛ Trying commit 88108bd with merge be80ce668cc66c6c4748400d6a1d7fe5bf440436...

bors · 2022-04-10T10:23:49Z

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

rust-timer · 2022-04-10T10:23:51Z

Queued be80ce668cc66c6c4748400d6a1d7fe5bf440436 with parent f7b4824, future comparison URL.

rust-timer · 2022-04-10T11:36:20Z

Finished benchmarking commit (be80ce668cc66c6c4748400d6a1d7fe5bf440436): comparison url.

Summary:

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

	Regressions 😿 (primary)	Regressions 😿 (secondary)	Improvements 🎉 (primary)	Improvements 🎉 (secondary)	All 😿 🎉 (primary)
count¹	24	27	0	0	24
mean²	0.3%	0.7%	N/A	N/A	0.3%
max	0.5%	1.3%	N/A	N/A	0.5%

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 ↩

cjgillot · 2022-04-17T07:47:11Z

This PR is the first in a series that should significantly reduce full-HIR traversal by queries. This is expected to bring improvements to incremental compilation, by decreasing result invalidation. This first PR creates the infrastructure for this change, as a new query hir_crate_items. This query does traverse the full HIR to record all definitions in order of appearance, so is quite slow to compute. The overall regression is 0.5% on primary benchmarks, this should be compensated when follow-up PRs land.
@rustbot label: +perf-regression-triaged

@bors r+

bors · 2022-04-17T07:47:13Z

📌 Commit 88108bd has been approved by cjgillot

bors · 2022-04-17T08:07:03Z

⌛ Testing commit 88108bd with merge edba282...

bors · 2022-04-17T10:31:46Z

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

rust-timer · 2022-04-17T11:45:18Z

Finished benchmarking commit (edba282): comparison url.

Summary:

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

	Regressions 😿 (primary)	Regressions 😿 (secondary)	Improvements 🎉 (primary)	Improvements 🎉 (secondary)	All 😿 🎉 (primary)
count¹	33	32	0	3	33
mean²	0.3%	0.7%	N/A	-1.0%	0.3%
max	0.4%	1.6%	N/A	-1.0%	0.4%

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 ↩

… r=cjgillot Refactor HIR item-like traversal (part 1) Issue rust-lang#95004 - Create hir_crate_items query which traverses tcx.hir_crate(()).owners to return a hir::ModuleItems - use tcx.hir_crate_items in tcx.hir().items() to return an iterator of hir::ItemId - use tcx.hir_crate_items to introduce a tcx.hir().par_items(impl Fn(hir::ItemId)) to traverse all items in parallel; Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com> cc `@cjgillot`

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 4, 2022

rust-highfive assigned petrochenkov Apr 4, 2022

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

cjgillot self-assigned this Apr 4, 2022

cjgillot reviewed Apr 4, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

petrochenkov assigned cjgillot and unassigned cjgillot and petrochenkov Apr 4, 2022

kckeiks force-pushed the create-hir-crate-items-query branch from b474ba5 to 292637c Compare April 5, 2022 02:13

This comment has been minimized.

Sign in to view

kckeiks force-pushed the create-hir-crate-items-query branch from 292637c to 4db16f0 Compare April 5, 2022 15:56

This comment has been minimized.

Sign in to view

kckeiks force-pushed the create-hir-crate-items-query branch from 4db16f0 to f213ed6 Compare April 5, 2022 17:34

This comment has been minimized.

Sign in to view

kckeiks force-pushed the create-hir-crate-items-query branch from f213ed6 to 6ef0f16 Compare April 5, 2022 18:54

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

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

kckeiks added 6 commits April 9, 2022 13:54

avoid accessing the interner by comparing the Symbol directly

51ee3d4

use copied() and avoid creating a vector in items and par_items

f983d26

use ItemId.def_id and avoid fetching Item

0b38596

update Finder to store LocalDefId

a349fc4

rename to par_for_each_item

a31632b

add comment about restriction of Target::from_def_kind

88108bd

kckeiks force-pushed the create-hir-crate-items-query branch from 36043bf to 88108bd Compare April 9, 2022 20:04

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

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 10, 2022

rustbot added the perf-regression-triaged The performance regression has been triaged. label Apr 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 Apr 17, 2022

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

bors merged commit edba282 into rust-lang:master Apr 17, 2022

rustbot added this to the 1.62.0 milestone Apr 17, 2022

kckeiks mentioned this pull request Jun 11, 2022

Refactor HIR item-like traversal #95004

Open

2 of 7 tasks

dwrensha mentioned this pull request Oct 6, 2022

Region parameter out of range when substituting in region #102751

Closed

Conversation

kckeiks commented Apr 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Apr 4, 2022

Uh oh!

rust-highfive commented Apr 4, 2022

Uh oh!

cjgillot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cjgillot Apr 4, 2022

Choose a reason for hiding this comment

Uh oh!

kckeiks Apr 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjgillot Apr 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kckeiks Apr 5, 2022

Choose a reason for hiding this comment

Uh oh!

cjgillot Apr 4, 2022

Choose a reason for hiding this comment

Uh oh!

kckeiks Apr 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

kckeiks commented Apr 5, 2022

Uh oh!

cjgillot commented Apr 5, 2022

Uh oh!

rust-timer commented Apr 5, 2022

Uh oh!

bors commented Apr 5, 2022

Uh oh!

bors commented Apr 5, 2022

Uh oh!

rust-timer commented Apr 5, 2022

Uh oh!

rust-timer commented Apr 5, 2022

Footnotes

Uh oh!

kckeiks commented Apr 6, 2022

Uh oh!

cjgillot commented Apr 10, 2022

Uh oh!

rust-timer commented Apr 10, 2022

Uh oh!

bors commented Apr 10, 2022

Uh oh!

bors commented Apr 10, 2022

Uh oh!

rust-timer commented Apr 10, 2022

Uh oh!

rust-timer commented Apr 10, 2022

Footnotes

Uh oh!

cjgillot commented Apr 17, 2022

Uh oh!

bors commented Apr 17, 2022

Uh oh!

bors commented Apr 17, 2022

Uh oh!

bors commented Apr 17, 2022

Uh oh!

rust-timer commented Apr 17, 2022

Footnotes

Uh oh!

Reviewers

Uh oh!

kckeiks commented Apr 4, 2022 •

edited

Loading

kckeiks Apr 4, 2022 •

edited

Loading

cjgillot Apr 5, 2022 •

edited

Loading

kckeiks Apr 4, 2022 •

edited

Loading