collector: move ensure_sufficient_stack out of the loop

[RalfJung]

According to the docs this call has some overhead to putting it inside the loop doesn't seem like a good idea.

r? @oli-obk

[RalfJung]

@bors try @rust-timer queue

[bors]

⌛ Trying commit 15f5307 with merge b089dda...

[bors]

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

[rust-timer]

Finished benchmarking commit (b089dda): comparison URL.

Overall result: ❌ regressions - 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 a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.4%	[1.4%, 1.4%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

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)	2.3%	[1.4%, 3.8%]	11
Regressions ❌ (secondary)	3.1%	[1.6%, 5.2%]	41
Improvements ✅ (primary)	-2.4%	[-2.4%, -2.4%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.0%	[-2.4%, 3.8%]	12

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.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.6%	[-3.5%, -2.1%]	9
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 666.919s -> 667.789s (0.13%)
Artifact size: 312.80 MiB -> 312.77 MiB (-0.01%)

[RalfJung]

Hm that's odd. Maybe spurious? We may not have a benchmark for a const that has a lot of relocations (or the overhead of ensure_sufficient_stack is actually not that bad).

FWIW I am also quite surprised by how collect_items_rec uses ensure_sufficient_stack... the main recursion path where collect_items_rec calls itself does not seem to have a stack growth check?

[RalfJung]

@bors try @rust-timer queue

[bors]

⌛ Trying commit ee746fb with merge 68f6ae9...

[bors]

☀️ Try build successful - checks-actions
Build commit: 68f6ae9 (68f6ae965ce763ddbc8aaad51d46dc468f513708)

[rust-timer]

Finished benchmarking commit (68f6ae9): comparison URL.

Overall result: no relevant changes - 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 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)	2.4%	[2.1%, 2.9%]	5
Regressions ❌ (secondary)	3.3%	[2.6%, 4.0%]	13
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.4%	[2.1%, 2.9%]	5

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.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.7%	[-2.5%, -0.9%]	2
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 668.793s -> 668.518s (-0.04%)
Artifact size: 312.78 MiB -> 312.73 MiB (-0.01%)

[compiler-errors]

r? compiler-errors @bors r+

[bors]

📌 Commit ee746fb has been approved by compiler-errors

It is now in the queue for this repository.

[bors]

⌛ Testing commit ee746fb with merge 62f98b4...

[bors]

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 62f98b4 to master...

[rust-timer]

Finished benchmarking commit (62f98b4): comparison URL.

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

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.3%	[0.3%, 0.3%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

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)	2.9%	[1.6%, 4.8%]	11
Regressions ❌ (secondary)	2.9%	[0.9%, 7.2%]	47
Improvements ✅ (primary)	-2.0%	[-2.0%, -2.0%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.5%	[-2.0%, 4.8%]	12

Cycles

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

Binary size

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

Bootstrap: 667.843s -> 669.037s (0.18%)
Artifact size: 312.81 MiB -> 312.76 MiB (-0.02%)

[rust-log-analyzer]

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

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

##[endgroup]
Starting download for Cargo-lock
Directory structure has been setup for the artifact
Total number of files that will be downloaded: 1
Artifact Cargo-lock was downloaded to /home/runner/work/rust/rust
##[group]Run actions/download-artifact@v3
with:
  name: cargo-updates
---
##[endgroup]
Starting download for cargo-updates
Directory structure has been setup for the artifact
Total number of files that will be downloaded: 1
Skipping download validation.
Artifact cargo-updates was downloaded to /home/runner/work/rust/rust
##[group]Run echo "${COMMIT_MESSAGE}" > commit.txt
echo "${COMMIT_MESSAGE}" > commit.txt
echo "${COMMIT_MESSAGE}" > commit.txt
cat cargo_update.log >> commit.txt

echo "${PR_MESSAGE}" > body.md
echo '```txt' >> body.md
cat cargo_update.log >> body.md
echo '```' >> body.md
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
  RUSTC_BOOTSTRAP: 1
  PR_TITLE: Weekly `cargo update`
  PR_MESSAGE: Automation to keep dependencies in `Cargo.lock` current.
following is the output from `cargo update`:
  COMMIT_MESSAGE: cargo update 
##[endgroup]
##[group]Run git config user.name github-actions
git config user.name github-actions
git config user.email [email protected]
git switch --force-create cargo_update
git add ./Cargo.lock
git commit --no-verify --file=commit.txt
---
##[endgroup]
Switched to a new branch 'cargo_update'
On branch cargo_update
Untracked files:
  (use "git add <file>..." to include in what will be committed)
 body.md
 commit.txt


nothing added to commit but untracked files present (use "git add" to track)
##[error]Process completed with exit code 1.

[klensy]

Still maxrss regression?

[RalfJung]

I have seen so many spurious maxrss regressions, I usually ignore that metric...