Remove usages of Unique from RawVec

[maxdexh]

View all comments

See zulip.

Probably needs a perf run, because it touches code that might be very sensitive to build times and debug mode

r? libs

[saethlin]

@bors try @rust-timer queue

[rust-bors]

💔 Test for 993bb96 failed: CI, CI

[hanna-kruppe]

@bors try

[rust-bors]

☀️ Try build successful (CI)
Build commit: b67dca9 (b67dca92df005a4ebbe8c3b45e6cfe97925fff26)
Base parent: 1936d44 (1936d44457352da43b2d740672b940ebc715ac38)

[hanna-kruppe]

Nice cleanup, I'd say "r=me if perf looks good" but we should probably do some due diligence about debug visualizers. You mentioned on Zulip that you tested manually in gdb, I can try on Windows locally later.

r? me

[hanna-kruppe]

Oh, huh, guess that a bunch of old backcompat hacks made the gdb visualizer resilient to this change:

rust/src/etc/gdb_providers.py

Line 18 in 1936d44

return ptr if ptr.type.code == gdb.TYPE_CODE_PTR else ptr[ptr.type.fields()[0]]

But the natvis looks like it’ll break:

rust/src/etc/natvis/liballoc.natvis

Line 10 in 1936d44

<ValuePointer>($T1*)buf.inner.ptr.pointer.pointer</ValuePointer>

[rust-bors]

💔 Test for aaf1dca failed: CI. Failed job:

• try - test-x86_64-msvc-1 (web logs, enhanced plaintext logs)

[maxdexh]

How do I run the failing benchmark locally?

Edit: Ah I got it, it also fails to compile locally with the same error ^^

[maxdexh]

Alright, I pulled back the auto trait impls from RawVec to RawVecInner, and that seems to have solved the issue. Really strange behavior from the trait solver considering these impls should all be equivalent.

Also fixed the natvis (i think)

[maxdexh]

Btw, the natvis field path for Rc looks incorrect too, but I'm not sure. Not touching that in this PR

[maxdexh]

Can you rerun rust-timer and the bors msvc job, please? ^^

Hoping to see no perf regressions this time

@rustbot ready

[hanna-kruppe]

@bors try @rust-timer queue

(will kick off the msvc tests after, don’t want to risk messing up the job set for doing everything at once)

[rust-bors]

☀️ Try build successful (CI)
Build commit: 026adb5 (026adb50e37fb6d6ee295cc05c52549b588eff95)
Base parent: 28e8a8c (28e8a8c81bf3b37909edac6c2a76e56f30cd492f)

[Kobzol]

(will kick off the msvc tests after, don’t want to risk messing up the job set for doing everything at once)

@hanna-kruppe That is fine, you can do a single try build with N different jobs. As long as the job set contains the x64 and aarch dist builds, perf. shoud work.

[hanna-kruppe]

Thanks for the confirmation . I figured it should work if I specified the right jobs, I just didn’t know which exact jobs perf needs and writing from my phone isn’t conductive to finding out and typing it correctly on the first try.

@bors try jobs=test-x86_64-msvc-*

[rust-timer]

Finished benchmarking commit (026adb5): 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%]	8
Improvements ✅ (secondary)	-0.2%	[-0.2%, -0.2%]	2
All ❌✅ (primary)	-0.2%	[-0.4%, -0.1%]	8

Max RSS (memory usage)

Results (primary 1.8%, secondary 1.7%)

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

	mean	range	count
Regressions ❌ (primary)	4.1%	[1.5%, 7.7%]	6
Regressions ❌ (secondary)	1.7%	[1.7%, 1.7%]	1
Improvements ✅ (primary)	-2.8%	[-3.2%, -2.2%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.8%	[-3.2%, 7.7%]	9

Cycles

Results (secondary 0.5%)

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

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

Binary size

Results (primary -0.2%, secondary 0.0%)

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.1%, 0.8%]	2
Regressions ❌ (secondary)	0.5%	[0.5%, 0.5%]	3
Improvements ✅ (primary)	-0.2%	[-0.5%, -0.0%]	33
Improvements ✅ (secondary)	-0.1%	[-0.1%, -0.0%]	11
All ❌✅ (primary)	-0.2%	[-0.5%, 0.8%]	35

Bootstrap: 496.137s -> 502.083s (1.20%)
Artifact size: 406.79 MiB -> 406.64 MiB (-0.04%)

[maxdexh]

Is the memory usage just noise? Those are some huge deltas. (Or maybe just next-solver being next-solver)

[theemathas]

Given that this broke our benchmarking code, will this PR need a crater run to make sure this isn't massively breaking something?

[maxdexh]

(please don't make me wait another week for crater)
The new version doesn't break benchmark code, and the regression was only on next-solver, so I would say no. But idk, not my call.

[rust-bors]

☀️ Try build successful (CI)
Build commit: a4f63d7 (a4f63d7dc07e9ef31a62a6ad04f7ab63694cf330)
Base parent: 28e8a8c (28e8a8c81bf3b37909edac6c2a76e56f30cd492f)