Move impl constness into impl trait header #134122

oli-obk · 2024-12-10T11:47:52Z

This PR is kind of the opposite of the rejected #134114

Instead of moving more things into the constness query, we want to keep them where their corresponding hir nodes are lowered. So I gave this a spin for impls, which have an obvious place to be (the impl trait header). And surprisingly it's also a perf improvement (likely just slightly better query & cache usage).

The issue was that removing anything from the constness query makes it just return NotConst, which is wrong. So I had to change it to bug! out if used wrongly, and only then remove the impl blocks from the constness query. I think this change is good in general, because it makes using constness more robust (as can be seen by how few sites that had to be changed, so it was almost solely used specifically for the purpose of asking for functions' constness). The main thing where this change was not great was in clippy, which was using the constness query as a general DefId -> constness map. I added a DefKind filter in front of that. If it becomes a more common pattern we can always move that helper into rustc.

rustbot · 2024-12-10T11:48:03Z

r? @petrochenkov

rustbot has assigned @petrochenkov.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot · 2024-12-10T11:48:05Z

Some changes occurred to the CTFE machinery

cc @rust-lang/wg-const-eval

rustbot · 2024-12-10T11:59:58Z

Some changes occurred to the CTFE machinery

cc @rust-lang/wg-const-eval

oli-obk · 2024-12-10T12:00:08Z

@bors try @rust-timer queue

Move impl constness into impl trait header The issue was that removing anything from the `constness` query makes it just return `NotConst`, which is wrong. So I had to change it to `bug!` out if used wrongly, and only then remove the impl blocks from the `constness` query.

bors · 2024-12-10T12:01:19Z

⌛ Trying commit 9498d5d with merge 45d653e...

bors · 2024-12-10T13:44:35Z

☀️ Try build successful - checks-actions
Build commit: 45d653e (45d653ecc300a0965f2d4aef8183a2ae331aff11)

rust-timer · 2024-12-10T16:34:09Z

Finished benchmarking commit (45d653e): 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-perf -perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.3%	[-1.2%, -0.1%]	40
Improvements ✅ (secondary)	-0.8%	[-2.5%, -0.2%]	39
All ❌✅ (primary)	-0.3%	[-1.2%, -0.1%]	40

Max RSS (memory usage)

Results (primary -1.2%, secondary -1.5%)

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.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.7%	[2.7%, 2.7%]	1
Improvements ✅ (primary)	-1.2%	[-1.2%, -1.2%]	2
Improvements ✅ (secondary)	-2.4%	[-3.3%, -1.2%]	5
All ❌✅ (primary)	-1.2%	[-1.2%, -1.2%]	2

Cycles

Results (primary -2.7%, secondary -2.9%)

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.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.7%	[-2.7%, -2.7%]	1
Improvements ✅ (secondary)	-2.9%	[-3.2%, -2.6%]	3
All ❌✅ (primary)	-2.7%	[-2.7%, -2.7%]	1

Binary size

Results (primary -0.0%, secondary 0.0%)

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.

	mean	range	count
Regressions ❌ (primary)	0.0%	[0.0%, 0.1%]	31
Regressions ❌ (secondary)	0.0%	[0.0%, 0.0%]	12
Improvements ✅ (primary)	-0.2%	[-0.3%, -0.2%]	8
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.0%	[-0.3%, 0.1%]	39

Bootstrap: 767.346s -> 766.945s (-0.05%)
Artifact size: 330.96 MiB -> 331.03 MiB (0.02%)

oli-obk · 2024-12-10T16:35:26Z

yay

petrochenkov · 2024-12-10T16:48:12Z

The implementation looks good to me, but from the description it's still not clear to me what is the motivation for this part

then remove the impl blocks from the constness query

(moving impl constness from one query to another).

Also one of the tests still hits the assert.

@rustbot author

rustbot · 2024-12-10T17:13:06Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

oli-obk · 2024-12-10T17:19:54Z

@rustbot ready

I added a motivation section.

The clippy test failure was actually a counterargument for this change, but since it's only a single clippy lint, I think the change in itself is still net positive.

petrochenkov · 2024-12-10T18:37:11Z

cc @fee1-dead as the reviewer of #134114.

@bors r+

bors · 2024-12-10T18:37:14Z

📌 Commit 7600696 has been approved by petrochenkov

It is now in the queue for this repository.

oli-obk · 2024-12-11T11:19:15Z

@bors r=petrochenkov

bors · 2024-12-11T11:19:18Z

📌 Commit 38642a6 has been approved by petrochenkov

It is now in the queue for this repository.

compiler/rustc_const_eval/src/const_eval/fn_queries.rs

oli-obk · 2024-12-12T13:09:53Z

@bors r-

oli-obk · 2024-12-12T20:06:42Z

@bors r=petrochenkov

bors · 2024-12-12T20:06:45Z

📌 Commit 2ffe3b1 has been approved by petrochenkov

It is now in the queue for this repository.

bors · 2024-12-13T16:17:38Z

⌛ Testing commit 2ffe3b1 with merge e217f94...

bors · 2024-12-13T19:10:24Z

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing e217f94 to master...

rust-timer · 2024-12-13T20:42:26Z

Finished benchmarking commit (e217f94): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Our benchmarks found a performance regression caused by this PR.
This might be an actual regression, but it can also be just noise.

Next Steps:

If the regression was expected or you think it can be justified,
please write a comment with sufficient written justification, and add
@rustbot label: +perf-regression-triaged to it, to mark the regression as triaged.
If you think that you know of a way to resolve the regression, try to create
a new PR with a fix for the regression.
If you do not understand the regression or you think that it is just noise,
you can ask the @rust-lang/wg-compiler-performance working group for help (members of this group
were already notified of this PR).

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	0.8%	[0.8%, 0.8%]	1
Regressions ❌ (secondary)	1.7%	[1.6%, 1.9%]	6
Improvements ✅ (primary)	-0.3%	[-1.1%, -0.1%]	59
Improvements ✅ (secondary)	-0.7%	[-2.5%, -0.2%]	41
All ❌✅ (primary)	-0.3%	[-1.1%, 0.8%]	60

Max RSS (memory usage)

Results (primary 2.4%, secondary -0.8%)

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.

	mean	range	count
Regressions ❌ (primary)	2.4%	[1.2%, 3.6%]	2
Regressions ❌ (secondary)	2.5%	[1.4%, 3.5%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.9%	[-2.5%, -1.4%]	6
All ❌✅ (primary)	2.4%	[1.2%, 3.6%]	2

Cycles

Results (secondary 2.3%)

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.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	7.8%	[7.3%, 8.8%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.2%	[-3.4%, -2.7%]	3
All ❌✅ (primary)	-	-	0

Binary size

Results (primary -0.0%, secondary 0.0%)

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.

	mean	range	count
Regressions ❌ (primary)	0.0%	[0.0%, 0.1%]	32
Regressions ❌ (secondary)	0.0%	[0.0%, 0.0%]	12
Improvements ✅ (primary)	-0.2%	[-0.3%, -0.2%]	8
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.0%	[-0.3%, 0.1%]	40

Bootstrap: 770.238s -> 769.384s (-0.11%)
Artifact size: 330.40 MiB -> 331.02 MiB (0.19%)

…enkov Move impl constness into impl trait header This PR is kind of the opposite of the rejected rust-lang#134114 Instead of moving more things into the `constness` query, we want to keep them where their corresponding hir nodes are lowered. So I gave this a spin for impls, which have an obvious place to be (the impl trait header). And surprisingly it's also a perf improvement (likely just slightly better query & cache usage). The issue was that removing anything from the `constness` query makes it just return `NotConst`, which is wrong. So I had to change it to `bug!` out if used wrongly, and only then remove the impl blocks from the `constness` query. I think this change is good in general, because it makes using `constness` more robust (as can be seen by how few sites that had to be changed, so it was almost solely used specifically for the purpose of asking for functions' constness). The main thing where this change was not great was in clippy, which was using the `constness` query as a general DefId -> constness map. I added a `DefKind` filter in front of that. If it becomes a more common pattern we can always move that helper into rustc.

Kobzol · 2024-12-23T14:05:13Z

More improvements than regressions.

@rustbot label: +perf-regression-triaged

…er-errors Spruce up the docs of several queries related to the type/trait system and const eval - Editorial - Proper rustdoc summary/synopsis line by making use of extra paragraphs: Leads to better rendered output on module pages, in search result lists and overall, too - Use rustdoc warning blocks for admonitions of the form "do not call / avoid calling this query directly" - Use intra-doc links of the form ``[`Self::$query`]`` to cross-link queries. Indeed, such links are generally a bit brittle due to the existence of `TyCtxtFeed` which only contains a subset of queries. Therefore the docs of `feedable` queries cannot cross-link to non-`feedable` ones. I'd say it's fine to use intra-doc links despite the potential/unlikely occasional future breakage (if a query with the aforementioned characteristics becomes `feedable`). `Self::` is nicer than `TyCtxt::` (which would be more stable) since it accounts for other contexts like `TyCtxt{Feed,At,Ensure{,WithValue}}` - Informative - Generally add, flesh out and correct some doc comments - Add *Panic* sections (to a few selected queries only). The lists of panics aren't necessarily exhaustive and focus on the more "obvious" or "important" panics. - Where applicable add a paragraph calling attention to the relevant [`#[rustc_*]` TEST attribute](https://rustc-dev-guide.rust-lang.org/compiler-debugging.html#rustc_-test-attributes) The one non-doc change (it's internal and not observable): Be even more defensive in `query constness`'s impl (spiritual follow-up to rust-lang#134122) (see self review comment). Fixes rust-lang#133494. r\? **any**(compiler-errors, oli-obk)

Rollup merge of rust-lang#134787 - fmease:spruce-up-queries, r=compiler-errors Spruce up the docs of several queries related to the type/trait system and const eval - Editorial - Proper rustdoc summary/synopsis line by making use of extra paragraphs: Leads to better rendered output on module pages, in search result lists and overall, too - Use rustdoc warning blocks for admonitions of the form "do not call / avoid calling this query directly" - Use intra-doc links of the form ``[`Self::$query`]`` to cross-link queries. Indeed, such links are generally a bit brittle due to the existence of `TyCtxtFeed` which only contains a subset of queries. Therefore the docs of `feedable` queries cannot cross-link to non-`feedable` ones. I'd say it's fine to use intra-doc links despite the potential/unlikely occasional future breakage (if a query with the aforementioned characteristics becomes `feedable`). `Self::` is nicer than `TyCtxt::` (which would be more stable) since it accounts for other contexts like `TyCtxt{Feed,At,Ensure{,WithValue}}` - Informative - Generally add, flesh out and correct some doc comments - Add *Panic* sections (to a few selected queries only). The lists of panics aren't necessarily exhaustive and focus on the more "obvious" or "important" panics. - Where applicable add a paragraph calling attention to the relevant [`#[rustc_*]` TEST attribute](https://rustc-dev-guide.rust-lang.org/compiler-debugging.html#rustc_-test-attributes) The one non-doc change (it's internal and not observable): Be even more defensive in `query constness`'s impl (spiritual follow-up to rust-lang#134122) (see self review comment). Fixes rust-lang#133494. r\? **any**(compiler-errors, oli-obk)

…er-errors Spruce up the docs of several queries related to the type/trait system and const eval - Editorial - Proper rustdoc summary/synopsis line by making use of extra paragraphs: Leads to better rendered output on module pages, in search result lists and overall, too - Use rustdoc warning blocks for admonitions of the form "do not call / avoid calling this query directly" - Use intra-doc links of the form ``[`Self::$query`]`` to cross-link queries. Indeed, such links are generally a bit brittle due to the existence of `TyCtxtFeed` which only contains a subset of queries. Therefore the docs of `feedable` queries cannot cross-link to non-`feedable` ones. I'd say it's fine to use intra-doc links despite the potential/unlikely occasional future breakage (if a query with the aforementioned characteristics becomes `feedable`). `Self::` is nicer than `TyCtxt::` (which would be more stable) since it accounts for other contexts like `TyCtxt{Feed,At,Ensure{,WithValue}}` - Informative - Generally add, flesh out and correct some doc comments - Add *Panic* sections (to a few selected queries only). The lists of panics aren't necessarily exhaustive and focus on the more "obvious" or "important" panics. - Where applicable add a paragraph calling attention to the relevant [`#[rustc_*]` TEST attribute](https://rustc-dev-guide.rust-lang.org/compiler-debugging.html#rustc_-test-attributes) The one non-doc change (it's internal and not observable): Be even more defensive in `query constness`'s impl (spiritual follow-up to rust-lang#134122) (see self review comment). Fixes rust-lang#133494. r\? **any**(compiler-errors, oli-obk)

rustbot assigned petrochenkov Dec 10, 2024

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 Dec 10, 2024

oli-obk force-pushed the push-zqnyznxtpnll branch from 75576e5 to 9498d5d Compare December 10, 2024 11:59

This comment has been minimized.

Sign in to view

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

This comment has been minimized.

Sign in to view

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

rustbot 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 Dec 10, 2024

oli-obk force-pushed the push-zqnyznxtpnll branch from 9498d5d to 7600696 Compare December 10, 2024 17:13

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 10, 2024

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 Dec 10, 2024

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 11, 2024

RalfJung reviewed Dec 11, 2024

View reviewed changes

compiler/rustc_const_eval/src/const_eval/fn_queries.rs Outdated Show resolved Hide resolved

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 12, 2024

Move impl constness into impl trait header

2ffe3b1

oli-obk force-pushed the push-zqnyznxtpnll branch from 38642a6 to 2ffe3b1 Compare December 12, 2024 20:06

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 12, 2024

bors mentioned this pull request Dec 13, 2024

Autodiff Upstreaming - rustc_codegen_llvm changes #130060

Merged

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 13, 2024

bors merged commit e217f94 into rust-lang:master Dec 13, 2024
7 checks passed

rustbot added this to the 1.85.0 milestone Dec 13, 2024

rustbot added the perf-regression Performance regression. label Dec 13, 2024

oli-obk deleted the push-zqnyznxtpnll branch December 13, 2024 22:49

rustbot added the perf-regression-triaged The performance regression has been triaged. label Dec 23, 2024

fmease mentioned this pull request Dec 26, 2024

Spruce up the docs of several queries related to the type/trait system and const eval #134787

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move impl constness into impl trait header #134122

Move impl constness into impl trait header #134122

oli-obk commented Dec 10, 2024 •

edited

Loading

rustbot commented Dec 10, 2024

rustbot commented Dec 10, 2024

rustbot commented Dec 10, 2024

oli-obk commented Dec 10, 2024

This comment has been minimized.

bors commented Dec 10, 2024

This comment has been minimized.

bors commented Dec 10, 2024

This comment has been minimized.

rust-timer commented Dec 10, 2024

oli-obk commented Dec 10, 2024

petrochenkov commented Dec 10, 2024

rustbot commented Dec 10, 2024

oli-obk commented Dec 10, 2024

petrochenkov commented Dec 10, 2024

bors commented Dec 10, 2024

oli-obk commented Dec 11, 2024

bors commented Dec 11, 2024

oli-obk commented Dec 12, 2024

oli-obk commented Dec 12, 2024

bors commented Dec 12, 2024

bors commented Dec 13, 2024

bors commented Dec 13, 2024

rust-timer commented Dec 13, 2024

Kobzol commented Dec 23, 2024

Move impl constness into impl trait header #134122

Move impl constness into impl trait header #134122

Conversation

oli-obk commented Dec 10, 2024 • edited Loading

rustbot commented Dec 10, 2024

rustbot commented Dec 10, 2024

rustbot commented Dec 10, 2024

oli-obk commented Dec 10, 2024

This comment has been minimized.

bors commented Dec 10, 2024

This comment has been minimized.

bors commented Dec 10, 2024

This comment has been minimized.

rust-timer commented Dec 10, 2024

Overall result: ✅ improvements - no action needed

oli-obk commented Dec 10, 2024

petrochenkov commented Dec 10, 2024

rustbot commented Dec 10, 2024

oli-obk commented Dec 10, 2024

petrochenkov commented Dec 10, 2024

bors commented Dec 10, 2024

oli-obk commented Dec 11, 2024

bors commented Dec 11, 2024

oli-obk commented Dec 12, 2024

oli-obk commented Dec 12, 2024

bors commented Dec 12, 2024

bors commented Dec 13, 2024

bors commented Dec 13, 2024

rust-timer commented Dec 13, 2024

Overall result: ❌✅ regressions and improvements - please read the text below

Kobzol commented Dec 23, 2024

oli-obk commented Dec 10, 2024 •

edited

Loading