[panstromek]

View all comments

Similar idea as #162535 and previous attempts.

Again, I also noticed that no test fails when I mess up the string, so I added a test case.

[panstromek]

perf test for field names of large structs. This probably won't be as visible, there's not that many of those. I'll try to pick only benchmarks that that have those + 2 other ones as a spot check.

@bors try
@rust-timer queue benchmarks=cargo-0.87.1,nalgebra-0.33.0,nalgebra-0.33.0-new-solver,eza-0.21.2,regex-automata-0.4.8,cranelift-codegen-0.119.0,hyper-1.6.0,image-0.25.6,ripgrep-14.1.1,ripgrep-14.1.1-nll,diesel-2.2.10,diesel-2.2.10-new-solver,tokio-1.53.1,await-call-tree,bitmaps-3.2.1

[rust-bors]

☀️ Try build successful (CI)
Build commit: 455d3ee (455d3ee7337cbd5399396321c912ae289397f52e)
Base parent: 2b548e4 (2b548e49fcdf52bed4446b3e245563681f5bc45d)

[rust-timer]

Finished benchmarking commit (455d3ee): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

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

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.2%	[-0.4%, -0.1%]	16
Improvements ✅ (secondary)	-0.3%	[-0.5%, -0.2%]	5
All ❌✅ (primary)	-0.2%	[-0.4%, -0.1%]	16

Max RSS (memory usage)

Results (primary -0.7%, secondary -2.4%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.5%	[0.4%, 0.6%]	4
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.3%	[-2.0%, -0.6%]	7
Improvements ✅ (secondary)	-2.4%	[-2.4%, -2.4%]	1
All ❌✅ (primary)	-0.7%	[-2.0%, 0.6%]	11

Cycles

Results (primary -0.7%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.5%	[0.5%, 0.6%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.9%	[-2.6%, -1.2%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.7%	[-2.6%, 0.6%]	4

Binary size

Results (primary -0.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.4%	[0.0%, 0.8%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.2%	[-0.4%, -0.0%]	22
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.1%	[-0.4%, 0.8%]	24

Bootstrap: missing data
Artifact size: 406.27 MiB -> 406.47 MiB (0.05%)

[panstromek]

Nice, looks better than I expected. This makes sense, the best result is ripgrep, which has two structs with 68 and 74 fields. That's where this helps the most.

[panstromek]

r? nnethercote

Are you ok if I keep assigning you these things or should I let rustbot roll the dice? I've been assigning you almost everything lately, but that's just because I work on things you implemented and I try to pick reviewer based on who worked on the relevant code.

[rustbot]

Changes to the code generated for builtin derived traits.

cc @nnethercote

[nnethercote]

Nice idea! Can you do the same thing for debug_struct_field[2345]_finish?

No problem with the reviews. I always ask an appropriate person directly instead of letting the random choice happen.

[panstromek]

Can you do the same thing for debug_struct_field[2345]_finish?

Oh, that's another question I have. I was going to try that in a separate PR to separate the impact, do you prefer to do all of them at once?

[nnethercote]

It's such a closely related change, might as well do it here. Can be a separate commit if you want.

[panstromek]

Let's see how this works out for smaller structs. I'm a bit worried tbh. I think this should be better as far as the generated code goes, because the MIR will be smaller, but there's also more Symbol manipulation and interning, so it might not be so clear cut.

@bors try @rust-timer queue

Note: I went a bit further and concatenated struct name with the fields as well. This was simpler to do in the debug_struct_field[2345]_finish at first so I did it for array based one as well.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[panstromek]

rebased

@rustbot ready

[nnethercote]

@bors r+

[rust-bors]

📌 Commit 4719ede has been approved by nnethercote

It is now in the queue for this repository.

[rust-log-analyzer]

The job test-x86_64-gnu-stable failed! Check out the build log: (web) (plain enhanced) (plain)

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

[rust-bors]

💔 Test for a5788b1 failed: CI. Failed job:

• auto - test-x86_64-gnu-stable (web logs, enhanced plaintext logs)

[tgross35]

@bors retry

Seems to be our spurious failure of the week

[tgross35]

Is this PR's history intentional? 15 commits with short descriptions seems like quite a lot, but as long as there isn't back-and-forth...

[panstromek]

Is this PR's history intentional?

Yes, I'm used to doing fairly granular atomic commits to separate changes from each other. But feel free to squash it with bors if it feels like too much.

[tgross35]

No worries, I'm a fan of atomic commits too. Just making sure since some of the cleanups were a bit small :)

[JonathanBrouwer]

@bors rollup=iffy
Making a rollup of perf-sensitive PRs