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.

Sign up for GitHub

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

Jump to bottom

avoid clone path prefix when lowering to hir #113847

Merged
merged 2 commits into from
Jul 21, 2023
Merged

avoid clone path prefix when lowering to hir #113847

merged 2 commits into from
Jul 21, 2023

Conversation

SparrowLii
Copy link
Member

@SparrowLii SparrowLii commented Jul 19, 2023 •
edited
Loading

Found this while trying to parallelize lower_to_hir.

When lowering to hir, Nested paths in ast will be split and the prefix segments will be cloned. This could be omited, since the only consequence is that the prefix segments in Paths in hir will have the same HirIds, and it seems harmless.

This simplifies the process of lowering to hir and avoids re-modification of ResolverAstLowering.

r? @Aaron1011
cc #99292

@SparrowLii
avoid clone path prefix when lowering to hir
c6f0a7c 
Signed-off-by: SparrowLii <liyuan179@huawei.com>
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 19, 2023
@Aaron1011
Copy link
Member

This looks good to me, but I'd like someone else to confirm that it's fine to have different segments with the same HirIds.

@SparrowLii
Copy link
Member Author

Thanks! @petrochenkov Could you help to have a look?

@cjgillot
Copy link
Contributor

Having multiple nodes with the same HirId is problematic in general.

In particular here, that does not happen.

prefix is lowered multiple times, but in different HIR owners (see with_hir_id_owner call). This call resets the node_id_to_local_id: NodeId -> HirId cache, so each segment gets renewed HirId in the new owner def_id.

For the first (use_tree, id) we get {use#0}.1, {use#0}.2, ... where {use#0} = local_def_id(id), and for the second (use_tree, id) we get {use#1}.1, {use#1}.2, ..., etc.

r=me with a comment

@SparrowLii
add comment for lower_use_tree
0377945 
Signed-off-by: SparrowLii <liyuan179@huawei.com>
@SparrowLii
Copy link
Member Author

SparrowLii commented Jul 19, 2023 •
edited
Loading

Thanks much for the explaination!

@Aaron1011 Can you help r=cjgillot? I'm not reviewer :)

@cjgillot
Copy link
Contributor

Thanks!
@bors r+

@bors
Copy link
Contributor

bors commented Jul 19, 2023

📌 Commit 0377945 has been approved by cjgillot

It is now in the queue for this repository.

@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 Jul 19, 2023
@GuillaumeGomez GuillaumeGomez mentioned this pull request Jul 19, 2023
Closed
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jul 19, 2023
@GuillaumeGomez
Rollup merge of rust-lang#113847 - SparrowLii:path_clone, r=cjgillot
f733127 
avoid clone path prefix when lowering to hir

Found this while trying to parallelize `lower_to_hir`.

When lowering to hir, `Nested` paths in `ast` will be split and the prefix segments will be cloned. This could be omited, since the only consequence is that the prefix segments in `Path`s in hir will have the same `HirId`s, and it seems harmless.

This simplifies the process of lowering to hir and avoids re-modification of `ResolverAstLowering`.

r? `@Aaron1011`
cc rust-lang#99292
@bors
Copy link
Contributor

bors commented Jul 20, 2023

⌛ Testing commit 0377945 with merge 15e819382021a6e76aaed69abff507eae5788cac...

@bors
Copy link
Contributor

bors commented Jul 20, 2023

💔 Test failed - checks-actions

@bors 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 Jul 20, 2023
@rust-log-analyzer
Copy link
Collaborator

The job dist-aarch64-linux failed! Check out the build log: (web) (plain)

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

@lqd
Copy link
Member

lqd commented Jul 20, 2023

@bors retry rust-lang/crates.io#6850

@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 Jul 20, 2023
@bors
Copy link
Contributor

bors commented Jul 21, 2023

⌛ Testing commit 0377945 with merge c06b2b9...

@bors
Copy link
Contributor

bors commented Jul 21, 2023

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 21, 2023
@bors bors merged commit c06b2b9 into rust-lang:master Jul 21, 2023
@rustbot rustbot added this to the 1.73.0 milestone Jul 21, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (c06b2b9): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 649.721s -> 650.093s (0.06%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Aaron1011 Aaron1011

Labels
merged-by-bors This PR was explicitly merged by bors. 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
Milestone
1.73.0
Development

Successfully merging this pull request may close these issues.
8 participants
@SparrowLii @Aaron1011 @cjgillot @bors @rust-log-analyzer @lqd @rust-timer @rustbot