Avoid code generation for ThinVec<Diagnostic>'s destructor in the query system #108359

Zoxc · 2023-02-22T19:42:13Z

This avoids 2 instances of the destructor of ThinVec<Diagnostic> from being included in execute_job. It also outlines the cold branch in store_side_effects / store_side_effects_for_anon_node.

rustbot · 2023-02-22T19:42:20Z

r? @TaKO8Ki

(rustbot has picked a reviewer for you, use r? to override)

TaKO8Ki · 2023-02-24T03:37:51Z

@bors try @rust-timer queue

bors · 2023-02-24T03:38:00Z

⌛ Trying commit e0e90b4828a0daf941235ae89d7e6e50a6acb481 with merge 0f61822f04de7a457545a4ba87c45e7a4bc101e1...

bors · 2023-02-24T06:06:43Z

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

rust-timer · 2023-02-24T08:15:30Z

Finished benchmarking commit (0f61822f04de7a457545a4ba87c45e7a4bc101e1): comparison URL.

Overall result: ❌ regressions - 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.

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

	mean	range	count
Regressions ❌ (primary)	1.2%	[1.2%, 1.2%]	1
Regressions ❌ (secondary)	2.3%	[2.0%, 2.8%]	7
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.2%	[1.2%, 1.2%]	1

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.

cjgillot · 2023-02-25T18:01:02Z

compiler/rustc_query_system/src/query/plumbing.rs


    if std::intrinsics::unlikely(!side_effects.is_empty()) {
        if Q::ANON {
            qcx.store_side_effects_for_anon_node(dep_node_index, side_effects);
        } else {
            qcx.store_side_effects(dep_node_index, side_effects);
        }
+    } else {
+        // Avoid generating code for the destructor here
+        mem::forget(side_effects);


This needs a comment to explain why it's not a memory leak.

cjgillot · 2023-02-26T10:41:07Z

The perf results don't show any improvement in instruction count nor bootstrap time. I wonder if this special case is worth it.

bors · 2023-04-30T01:11:06Z

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

JohnCSimon · 2023-11-12T18:23:59Z

@Zoxc
ping from triage - can you post your status on this PR? There hasn't been an update in a few months and you have merge conflicts.

FYI: when a PR is ready for review, send a message containing
@rustbot ready to switch to S-waiting-on-review so the PR is in the reviewer's backlog.

Zoxc · 2023-11-14T18:08:38Z

It's blocked on Gankra/thin-vec#46 to make it cleaner.

rustbot · 2023-12-07T14:42:53Z

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

Zoxc · 2023-12-07T14:44:25Z

I've made the implementation a bit cleaner using has_capacity.

Zoxc · 2023-12-07T14:44:42Z

@rustbot ready

Zoxc · 2024-01-18T06:54:22Z

@rustbot ready

cjgillot · 2024-01-20T12:29:15Z

compiler/rustc_query_system/src/query/mod.rs

+    pub fn maybe_any(&self) -> bool {
+        // Use `has_capacity` so that the destructor for `self.diagnostics` can be skipped
+        // if `maybe_any` is known to be false.
+        self.diagnostics.has_capacity()


Could you keep the destructuring assignment? That's a good way to ensure we don't forget anything in a refactor.

…ry system

cjgillot · 2024-01-20T13:51:29Z

@bors r+

bors · 2024-01-20T13:51:32Z

📌 Commit 862011e has been approved by cjgillot

It is now in the queue for this repository.

bors · 2024-01-20T15:20:18Z

⌛ Testing commit 862011e with merge 159bdc1...

bors · 2024-01-20T17:32:50Z

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

rust-timer · 2024-01-20T19:01:00Z

Finished benchmarking commit (159bdc1): 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)

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.

	mean	range	count
Regressions ❌ (primary)	3.0%	[2.8%, 3.1%]	2
Regressions ❌ (secondary)	3.0%	[2.5%, 3.9%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.0%	[-2.7%, -1.3%]	4
All ❌✅ (primary)	3.0%	[2.8%, 3.1%]	2

Cycles

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

Binary size

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.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.4%	[-0.5%, -0.4%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.4%	[-0.5%, -0.4%]	3

Bootstrap: 666.038s -> 665.65s (-0.06%)
Artifact size: 308.34 MiB -> 308.31 MiB (-0.01%)

rustbot assigned TaKO8Ki Feb 22, 2023

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 Feb 24, 2023

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Feb 24, 2023

cjgillot reviewed Feb 25, 2023

View reviewed changes

Zoxc force-pushed the side-effects-tweak branch from e0e90b4 to cfa18e5 Compare February 26, 2023 00:33

Zoxc mentioned this pull request Feb 27, 2023

Add has_capacity as a proxy for is_singleton Gankra/thin-vec#46

Merged

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 Apr 15, 2023

cjgillot assigned cjgillot and unassigned TaKO8Ki Sep 25, 2023

Zoxc force-pushed the side-effects-tweak branch from cfa18e5 to adef9e7 Compare December 7, 2023 14:42

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 7, 2023

This comment has been minimized.

Sign in to view

Zoxc force-pushed the side-effects-tweak branch from adef9e7 to 5dad014 Compare December 7, 2023 19:27

This comment has been minimized.

Sign in to view

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 Dec 17, 2023

Zoxc force-pushed the side-effects-tweak branch from 5dad014 to 53cf215 Compare January 18, 2024 05:39

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 Jan 18, 2024

cjgillot reviewed Jan 20, 2024

View reviewed changes

Avoid code generation for ThinVec<Diagnostic>'s destructor in the que…

862011e

…ry system

Zoxc force-pushed the side-effects-tweak branch from 53cf215 to 862011e Compare January 20, 2024 12:46

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 Jan 20, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 20, 2024

bors merged commit 159bdc1 into rust-lang:master Jan 20, 2024
12 checks passed

rustbot added this to the 1.77.0 milestone Jan 20, 2024

rustbot removed the perf-regression Performance regression. label Jan 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid code generation for ThinVec<Diagnostic>'s destructor in the query system #108359

Avoid code generation for ThinVec<Diagnostic>'s destructor in the query system #108359

Zoxc commented Feb 22, 2023

rustbot commented Feb 22, 2023

TaKO8Ki commented Feb 24, 2023

This comment has been minimized.

bors commented Feb 24, 2023

bors commented Feb 24, 2023

This comment has been minimized.

rust-timer commented Feb 24, 2023

cjgillot Feb 25, 2023

Zoxc Feb 26, 2023

cjgillot commented Feb 26, 2023

bors commented Apr 30, 2023

JohnCSimon commented Nov 12, 2023

Zoxc commented Nov 14, 2023

rustbot commented Dec 7, 2023

Zoxc commented Dec 7, 2023

Zoxc commented Dec 7, 2023

This comment has been minimized.

This comment has been minimized.

Zoxc commented Jan 18, 2024

cjgillot Jan 20, 2024

cjgillot commented Jan 20, 2024

bors commented Jan 20, 2024

bors commented Jan 20, 2024

bors commented Jan 20, 2024

rust-timer commented Jan 20, 2024

Avoid code generation for ThinVec<Diagnostic>'s destructor in the query system #108359

Avoid code generation for ThinVec<Diagnostic>'s destructor in the query system #108359

Conversation

Zoxc commented Feb 22, 2023

rustbot commented Feb 22, 2023

TaKO8Ki commented Feb 24, 2023

This comment has been minimized.

bors commented Feb 24, 2023

bors commented Feb 24, 2023

This comment has been minimized.

rust-timer commented Feb 24, 2023

Overall result: ❌ regressions - ACTION NEEDED

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjgillot commented Feb 26, 2023

bors commented Apr 30, 2023

JohnCSimon commented Nov 12, 2023

Zoxc commented Nov 14, 2023

rustbot commented Dec 7, 2023

Zoxc commented Dec 7, 2023

Zoxc commented Dec 7, 2023

This comment has been minimized.

This comment has been minimized.

Zoxc commented Jan 18, 2024

Choose a reason for hiding this comment

cjgillot commented Jan 20, 2024

bors commented Jan 20, 2024

bors commented Jan 20, 2024

bors commented Jan 20, 2024

rust-timer commented Jan 20, 2024

Overall result: no relevant changes - no action needed