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

Don't explicitly track crate_name getters #85837

Closed
wants to merge 1 commit into from
Closed

Don't explicitly track crate_name getters #85837

wants to merge 1 commit into from

Conversation

bjorn3
Copy link
Member

@bjorn3 bjorn3 commented May 30, 2021

cc #85804 (comment)

Both require a CrateNum as argument whose identity depends on both the stable crate id and thus also the crate name.

@rust-highfive
Copy link
Collaborator

r? @estebank

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 30, 2021
@bjorn3 bjorn3 mentioned this pull request May 30, 2021
Merged
@cjgillot
Copy link
Contributor

The crate_name can also be made a plain function.

@bjorn3
Copy link
Member Author

bjorn3 commented May 30, 2021

Huh, it seems that both crate_name and original_crate_name are defined as cdata.root.name for extern crates and as tcx.crate_name for the local crate. Why is there a split between the two?

@petrochenkov
Copy link
Contributor

@bjorn3

Why is there a split between the two?

Historical reasons, the original_* variant is a leftover from a past technical debt and can be removed.

@cjgillot
Copy link
Contributor

I assume this is a historical artefact. #85153 already proposes to drop original_crate_name in favor of the other.

@bjorn3
Copy link
Member Author

bjorn3 commented May 31, 2021

I will wait for that PR to land first then.

@bjorn3 bjorn3 added the S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. label May 31, 2021
@bors
Copy link
Contributor

bors commented Jun 1, 2021

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

@crlf0710 crlf0710 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: Marked as blocked ❌ on something else such as an RFC or other implementation work. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 26, 2021
@crlf0710
Copy link
Member

This seems unblocked now.

@bjorn3
Copy link
Member Author

bjorn3 commented Jun 30, 2021

Rebased

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bjorn3
Copy link
Member Author

bjorn3 commented Jul 1, 2021

@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 Jul 1, 2021
@bors
Copy link
Contributor

bors commented Jul 1, 2021

⌛ Trying commit 7e0702a9b0a8325b5d79cc6a86006e27dd862967 with merge be36ebb2d50e265c3c4ea8fad5c6c37c5dd418a6...

@bors
Copy link
Contributor

bors commented Jul 1, 2021

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

@rust-timer
Copy link
Collaborator

Queued be36ebb2d50e265c3c4ea8fad5c6c37c5dd418a6 with parent 64de497, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (be36ebb2d50e265c3c4ea8fad5c6c37c5dd418a6): comparison url.

Summary: This change led to significant improvements 🎉 in compiler performance.

  • Moderate improvement in instruction counts (up to -2.6% on full builds of externs-debug)

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. 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 -perf-regression

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jul 1, 2021
@JohnCSimon JohnCSimon removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 18, 2021
@bjorn3 bjorn3 changed the title Don't explicitly track crate_name and stable_crate_id getters Don't explicitly track crate_name getters Aug 8, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bjorn3
Copy link
Member Author

bjorn3 commented Aug 8, 2021

@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 Aug 8, 2021
@bors
Copy link
Contributor

bors commented Aug 8, 2021

⌛ Trying commit 4733317 with merge 3e5d3e8875ba1e9b8e064fd35fd8514aa88e694a...

@bors
Copy link
Contributor

bors commented Aug 8, 2021

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

@rust-timer
Copy link
Collaborator

Queued 3e5d3e8875ba1e9b8e064fd35fd8514aa88e694a with parent c4c2986, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (3e5d3e8875ba1e9b8e064fd35fd8514aa88e694a): comparison url.

Summary: This benchmark run did not return any significant changes.

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.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Aug 8, 2021
@bjorn3
Copy link
Member Author

bjorn3 commented Aug 8, 2021

This is a slight regression.

@bjorn3 bjorn3 closed this Aug 8, 2021
@bjorn3 bjorn3 deleted the cstore_refactoring branch August 8, 2021 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjgillot cjgillot cjgillot left review comments

Assignees

@estebank estebank

Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.