Skip to content

Ensure that queries only return Copy types. #93511

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
Merged
merged 6 commits into from
Feb 10, 2022
Merged

Ensure that queries only return Copy types. #93511

merged 6 commits into from
Feb 10, 2022

Conversation

cjgillot
Copy link
Contributor

This should pervent the perf footgun of returning a result with an expensive Clone impl (like a Vec of a hash map).

I went for the stupid solution of allocating on an arena everything that was not Copy. Some query results could be made Copy easily, but I did not really investigate.

@rust-highfive
Copy link
Contributor

Some changes occured to rustc_codegen_cranelift

cc @bjorn3

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occured to rustc_codegen_gcc

cc @antoyo

@rustbot 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 Jan 31, 2022
@rust-highfive
Copy link
Contributor

r? @wesleywiser

(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 Jan 31, 2022
@cjgillot
Copy link
Contributor Author

@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 Jan 31, 2022
@bors
Copy link
Collaborator

bors commented Jan 31, 2022

⌛ Trying commit a5ca17e836e29e0a4281c5c9eae26ed2dc6eb64f with merge 3f8b0271187f685af17c242eca5d5404bf87cd8b...

@bors
Copy link
Collaborator

bors commented Jan 31, 2022

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

@rust-timer
Copy link
Collaborator

Queued 3f8b0271187f685af17c242eca5d5404bf87cd8b with parent 24b8bb1, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (3f8b0271187f685af17c242eca5d5404bf87cd8b): comparison url.

Summary: This benchmark run shows 20 relevant improvements 🎉 to instruction counts.

  • Average relevant improvement: -2.1%
  • Largest improvement in instruction counts: -6.1% on incr-unchanged builds of externs opt

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.

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

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 1, 2022
@oli-obk
Copy link
Contributor

oli-obk commented Feb 1, 2022

Amazing. Some cleanup ideas, but tbh, this lgtm, so whatever you feel like :)

@cjgillot
Copy link
Contributor Author

cjgillot commented Feb 1, 2022

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

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

@bors
Copy link
Collaborator

bors commented Feb 1, 2022

@cjgillot: 🔑 Insufficient privileges: not in try users

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 1, 2022
@oli-obk
Copy link
Contributor

oli-obk commented Feb 1, 2022

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

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

@bors
Copy link
Collaborator

bors commented Feb 1, 2022

⌛ Trying commit fd9778b6c1dbd5391bd0fe926f80dfcc5f37db35 with merge 6a4cc1f5ef07bdc1d8370ad2566352a47464d027...

@oli-obk
Copy link
Contributor

oli-obk commented Feb 1, 2022 •
edited
Loading

@cjgillot: 🔑 Insufficient privileges: not in try users

What uh... gimme a bit, we'll have that sorted out

Uh... I have no clue, if it fails again, ping infra I guess?

@cjgillot
Copy link
Contributor Author

cjgillot commented Feb 9, 2022

@bors r=oli-obk

@bors
Copy link
Collaborator

bors commented Feb 9, 2022

📌 Commit 8edd32c has been approved by oli-obk

@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 Feb 9, 2022
@bors
Copy link
Collaborator

bors commented Feb 9, 2022

⌛ Testing commit 8edd32c with merge 4bf1f1cdf536cff794864a3bf90640de0dd9b058...

@bors
Copy link
Collaborator

bors commented Feb 10, 2022

💥 Test timed out

@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 Feb 10, 2022
@rust-log-analyzer
Copy link
Collaborator

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

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

@oli-obk
Copy link
Contributor

oli-obk commented Feb 10, 2022

@bors retry timeout

@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 Feb 10, 2022
@bors
Copy link
Collaborator

bors commented Feb 10, 2022

⌛ Testing commit 8edd32c with merge 56cd04a...

@oli-obk oli-obk assigned oli-obk and unassigned wesleywiser Feb 10, 2022
@bors
Copy link
Collaborator

bors commented Feb 10, 2022

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 56cd04a to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 10, 2022
@bors bors merged commit 56cd04a into rust-lang:master Feb 10, 2022
@rustbot rustbot added this to the 1.60.0 milestone Feb 10, 2022
@bors bors mentioned this pull request Feb 10, 2022
Merged
@cjgillot cjgillot deleted the query-copy branch February 10, 2022 13:49
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (56cd04a): comparison url.

Summary: This benchmark run shows 15 relevant improvements 🎉 to instruction counts.

  • Average relevant improvement: -2.3%
  • Largest improvement in instruction counts: -6.1% on incr-unchanged builds of externs debug

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

@rustbot label: -perf-regression

bjorn3 pushed a commit to bjorn3/rust that referenced this pull request Feb 26, 2022
@bors
Auto merge of rust-lang#93511 - cjgillot:query-copy, r=oli-obk
22a3068 
Ensure that queries only return Copy types.

This should pervent the perf footgun of returning a result with an expensive `Clone` impl (like a `Vec` of a hash map).

I went for the stupid solution of allocating on an arena everything that was not `Copy`. Some query results could be made Copy easily, but I did not really investigate.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oli-obk oli-obk oli-obk left review comments

@deltragon deltragon deltragon left review comments

Assignees

@oli-obk oli-obk

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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.60.0
Development

Successfully merging this pull request may close these issues.
9 participants
@cjgillot @rust-highfive @rust-timer @bors @oli-obk @rust-log-analyzer @deltragon @wesleywiser @rustbot