Use LocalDefId as an index in metadata encoding

[aerooneqq]

View all comments

The goal of this PR to eliminate encoding of a raw DefIndex, instead we should encode something (LocalDefId) that certainly indicates that index belongs to the local crate. The implementation of DefIndex encoding should always panic to prevent accidental remappings of non-local DefIndexes in #162809.

So the plan is as follows:

• Use LocalDefId as a writing index in tables for metadata encoding, use LocalDefId instead of DefIndex everywhere, specialize encode/decode methods to correctly process it,
• Replace implementation of encode_def_index in EncodeContext with panic, so we never remap and encode def index,
• Make tables' keys two-sided: we can read and write using different index types, in this case we will write using LocalDefId and read using DefIndex (done in Allow using different index types when reading and writing to tables #163447).

Blocked by #162900.

r? @petrochenkov

[petrochenkov]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 2fa5a47 (2fa5a472bd28122b86b69706e673d2b3704e5c27)
Base parent: 4e701dc (4e701dc6ba63a40c908a173df269a55e1fd6297e)

[rust-timer]

Finished benchmarking commit (2fa5a47): comparison URL.

Overall result: ❌ regressions - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@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.8%	[0.8%, 0.9%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

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)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.5%	[-0.5%, -0.5%]	1
All ❌✅ (primary)	-	-	0

Cycles

Results (primary -1.6%, secondary 3.8%)

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.8%	[3.8%, 3.8%]	1
Improvements ✅ (primary)	-1.6%	[-1.6%, -1.6%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.6%	[-1.6%, -1.6%]	1

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 488.984s -> 488.935s (-0.01%)
Artifact size: 406.31 MiB -> 406.32 MiB (0.00%)

[aerooneqq]

After some experiments with LocalDefIndex and LocalDefId it seems that LocalDefId is a better option, so now waiting for #162900 to merge and after that this will be ready.

[rust-bors]

🔨 3 commits were squashed into b92cb3e.

[aerooneqq]

@bors r=oli-obk,petrochenkov

[rust-bors]

📌 Commit b92cb3e has been tentatively approved by oli-obk,petrochenkov

It will be put into the queue for this repository once PR CI succeeds.

[rust-log-analyzer]

The job dist-aarch64-freebsd 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 fcbcce8 failed: CI. Failed job:

• auto - dist-aarch64-freebsd (web logs, enhanced plaintext logs)

[JonathanBrouwer]

@bors retry

[rust-bors]

☀️ Test successful - CI
Approved by: oli-obk,petrochenkov
Duration: 3h 15m 21s
Pushing 65efa19 to main...

[github-actions]

What is this?

This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing fc2324d (parent) -> 65efa19 (this PR)

Test differences

Show 6 test diffs

6 doctest diffs were found. These are ignored, as they are noisy.

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard 65efa194201c67ba29b7384255a534d61c2aa3cb --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

1. test-x86_64-msvc-ext2: 1h 1m -> 1h 50m (+81.2%)
2. test-armhf-gnu: 1h 1m -> 1h 40m (+64.3%)
3. test-x86_64-mingw-2: 1h 43m -> 2h 48m (+63.0%)
4. test-x86_64-mingw-1: 1h 58m -> 3h 2m (+54.5%)
5. test-x86_64-gnu-tools: 47m 53s -> 1h 10m (+48.1%)
6. test-x86_64-gnu-miri: 1h 6m -> 1h 34m (+42.7%)
7. dist-android: 22m 45s -> 32m 26s (+42.6%)
8. dist-apple-various: 2h -> 2h 48m (+39.6%)
9. test-i686-gnu-1: 2h 23m -> 1h 29m (-37.7%)
10. test-x86_64-gnu-next-trait-solver-polonius: 1h 5m -> 41m 5s (-37.0%)

How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

[rust-timer]

Finished benchmarking commit (65efa19): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This perf run didn't have relevant results for this metric.

Max RSS (memory usage)

Results (secondary 2.6%)

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)	2.6%	[0.7%, 3.7%]	5
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

Results (secondary 2.8%)

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)	2.8%	[2.7%, 3.0%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 484.956s -> 485.904s (0.20%)
Artifact size: 406.45 MiB -> 406.44 MiB (-0.00%)