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.

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

Rewrite representability #100720

Merged
merged 1 commit into from
Oct 8, 2022
Merged

Rewrite representability #100720

merged 1 commit into from
Oct 8, 2022

Conversation

camsteffen
Copy link
Contributor

@camsteffen camsteffen commented Aug 18, 2022

  • Improve placement of Box in the suggestion
  • Multiple items in a cycle emit 1 error instead of an error for each item in the cycle
  • Introduce representability query to avoid traversing an item every time it is used.
  • Also introduce params_in_repr query to avoid traversing generic items every time it is used.

@rust-highfive
Copy link
Collaborator

r? @fee1-dead

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

@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 Aug 18, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 18, 2022
@rust-log-analyzer

This comment has been minimized.

@camsteffen
Copy link
Contributor Author

camsteffen commented Aug 18, 2022

Ope I'm getting a cycle error in an incremental test. Not sure how to fix that but I'm sure it has to do with my usage of ImplicitCtxt...

Edit: Fixed with eval_always.

@camsteffen
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 Aug 18, 2022
@bors
Copy link
Contributor

bors commented Aug 18, 2022

⌛ Trying commit 74b5ebb844d94d9f73b616935db85fcf10cee785 with merge 44959bb541376c812edaad691a6f425d303af4cc...

@cjgillot
Copy link
Contributor

Why does the output change without eval_always?

@camsteffen
Copy link
Contributor Author

camsteffen commented Aug 18, 2022

@cjgillot IIUC, eval_always is necessary because the query uses "global state" in ImplicitCtxt, so a nested query can't properly resume where it left off in incremental. I think it would be okay to cache only results that are Ok, but I don't know if it's possible to cache based on the result value?

@bors
Copy link
Contributor

bors commented Aug 18, 2022

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

@rust-timer
Copy link
Collaborator

Queued 44959bb541376c812edaad691a6f425d303af4cc with parent 8064a49, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (44959bb541376c812edaad691a6f425d303af4cc): comparison url.

Instruction count

  • Primary benchmarks: ❌ relevant regressions found
  • Secondary benchmarks: ❌ relevant regressions found
mean1 max count2
Regressions ❌
(primary)
0.6% 1.4% 123
Regressions ❌
(secondary)
0.9% 1.7% 72
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.2% -0.3% 4
All ❌✅ (primary) 0.6% 1.4% 123

Max RSS (memory usage)

Results
  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: mixed results
mean1 max count2
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.7% 3.3% 3
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.0% -2.7% 2
All ❌✅ (primary) - - 0

Cycles

Results
  • Primary benchmarks: mixed results
  • Secondary benchmarks: ❌ relevant regressions found
mean1 max count2
Regressions ❌
(primary)
2.3% 2.9% 3
Regressions ❌
(secondary)
2.9% 3.7% 6
Improvements ✅
(primary)
-2.0% -2.0% 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 1.2% 2.9% 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 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

Footnotes

  1. the arithmetic mean of the percent change 2 3

  2. number of relevant changes 2 3

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Aug 18, 2022
@camsteffen
Copy link
Contributor Author

Hmm is there some combination of query modifiers that might get the perf back? Or is my whole approach just flawed?

@fee1-dead
Copy link
Member

r? compiler

I'm not familiar enough with query code and representability to review this.

@rust-highfive rust-highfive assigned cjgillot and unassigned fee1-dead Aug 19, 2022
@cjgillot
Copy link
Contributor

By marking the query as eval_always, you force recomputation of all representability, even if nothing has changed. This causes the regression in incr-unchanged and incr-patched profiles. You also have a regression in the full profiles, but I'm not sure where this comes from.

IIUC, the only case a type is not representable, is when there is a cycle in its representation. I'd suggest a 2 step approach without global state:

  1. one query to list the local ADTs that appear in the representability, returning a vector of field ids & ADT's id;
  2. the toplevel query is_representable that performs a graph traversal to find a cycle.

Caching the graph traversal in 2 requires special support from the query system. I have a commit that would implement that, but I'm still trying to design what the API should look like.

@camsteffen
Copy link
Contributor Author

Caching the graph traversal in 2 requires special support from the query system. I have a commit that would implement that, but I'm still trying to design what the API should look like.

So you think we can cache non-acyclic query results? Sounds fun!

Copy link
Contributor Author

@camsteffen camsteffen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noting a couple problems I found with my own code fwiw 😆

compiler/rustc_ty_utils/src/representability.rs Outdated Show resolved Hide resolved
compiler/rustc_ty_utils/src/representability.rs Outdated Show resolved Hide resolved
@cjgillot 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 Aug 31, 2022
@camsteffen
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

@bors
Copy link
Contributor

bors commented Oct 4, 2022

⌛ Trying commit ab8509ee3eda951741269715e55855f5eadf4605 with merge 1a1e80337e2850a979b64f8fe9c654fbe79accf8...

@bors
Copy link
Contributor

bors commented Oct 4, 2022

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

@rust-timer
Copy link
Collaborator

Queued 1a1e80337e2850a979b64f8fe9c654fbe79accf8 with parent 01af504, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (1a1e80337e2850a979b64f8fe9c654fbe79accf8): comparison URL.

Overall result: ✅ improvements - no action needed

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.

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

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.7% [-1.0%, -0.4%] 2
Improvements ✅
(secondary)
-0.4% [-0.6%, -0.4%] 7
All ❌✅ (primary) -0.7% [-1.0%, -0.4%] 2

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)
8.3% [0.9%, 15.8%] 4
Regressions ❌
(secondary)
2.4% [1.9%, 3.2%] 4
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-4.1% [-8.7%, -2.1%] 4
All ❌✅ (primary) 8.3% [0.9%, 15.8%] 4

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)
2.7% [2.7%, 2.8%] 2
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.7% [2.7%, 2.8%] 2

Footnotes

  1. the arithmetic mean of the percent change 2 3

  2. number of relevant changes 2 3

@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. perf-regression Performance regression. labels Oct 5, 2022
@rust-log-analyzer

This comment has been minimized.

@cjgillot
Copy link
Contributor

cjgillot commented Oct 8, 2022

Thanks @camsteffen for accepting to re-do it multiple times.
@bors r+

@bors
Copy link
Contributor

bors commented Oct 8, 2022

📌 Commit ff940db 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. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 8, 2022
@bors
Copy link
Contributor

bors commented Oct 8, 2022

⌛ Testing commit ff940db with merge bba9785...

@bors
Copy link
Contributor

bors commented Oct 8, 2022

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 8, 2022
@bors bors merged commit bba9785 into rust-lang:master Oct 8, 2022
@rustbot rustbot added this to the 1.66.0 milestone Oct 8, 2022
@camsteffen camsteffen deleted the representable branch October 8, 2022 15:33
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (bba9785): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.4% [-0.8%, -0.2%] 38
Improvements ✅
(secondary)
-0.9% [-3.3%, -0.2%] 21
All ❌✅ (primary) -0.4% [-0.8%, -0.2%] 38

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)
8.8% [2.4%, 15.8%] 4
Regressions ❌
(secondary)
4.0% [2.1%, 5.2%] 4
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.5% [-4.6%, -1.2%] 7
All ❌✅ (primary) 8.8% [2.4%, 15.8%] 4

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.6% [1.9%, 5.3%] 9
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Footnotes

  1. the arithmetic mean of the percent change 2 3

  2. number of relevant changes 2 3

Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
Rewrite representability

 * Improve placement of `Box` in the suggestion
 * Multiple items in a cycle emit 1 error instead of an error for each item in the cycle
 * Introduce `representability` query to avoid traversing an item every time it is used.
 * Also introduce `params_in_repr` query to avoid traversing generic items every time it is used.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) 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
Development

Successfully merging this pull request may close these issues.

9 participants