Conversation
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Create `LocalDefIndex` for metadata encoding
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (2fa5a47): comparison URL. Overall result: ❌ regressions - no action neededBenchmarking 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 countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
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.
CyclesResults (primary -1.6%, secondary 3.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 488.984s -> 488.935s (-0.01%) |
3b195e0 to
2b5f330
Compare
This comment has been minimized.
This comment has been minimized.
2b5f330 to
012aa52
Compare
…ata, r=petrochenkov Allow using different index types when reading and writing to tables Make tables of metadata two-sided: one can write with one index type and read with another, as long as both those types are indexes. That will be used in rust-lang#163321 when we will have `LocalDefIndex` or similar type. r? @petrochenkov
…ata, r=petrochenkov Allow using different index types when reading and writing to tables Make tables of metadata two-sided: one can write with one index type and read with another, as long as both those types are indexes. That will be used in rust-lang#163321 when we will have `LocalDefIndex` or similar type. r? @petrochenkov
…ata, r=petrochenkov Allow using different index types when reading and writing to tables Make tables of metadata two-sided: one can write with one index type and read with another, as long as both those types are indexes. That will be used in rust-lang#163321 when we will have `LocalDefIndex` or similar type. r? @petrochenkov
…ata, r=petrochenkov Allow using different index types when reading and writing to tables Make tables of metadata two-sided: one can write with one index type and read with another, as long as both those types are indexes. That will be used in rust-lang#163321 when we will have `LocalDefIndex` or similar type. r? @petrochenkov
This comment has been minimized.
This comment has been minimized.
d3319be to
68c2396
Compare
68c2396 to
52b92f8
Compare
This comment has been minimized.
This comment has been minimized.
|
After some experiments with |
LocalDefIndex for metadata encodingLocalDefId as an index in metadata encoding
This comment has been minimized.
This comment has been minimized.
310ab96 to
9f82b55
Compare
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Use `LocalDefId` as an index in metadata encoding
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (9c851b5): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking 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 Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)This perf run didn't have relevant results for this metric. CyclesResults (secondary 4.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 491.121s -> 491.415s (0.06%) |
| } | ||
| } | ||
|
|
||
| impl<'a, 'tcx> Decodable<MetadataDecodeContext<'a, 'tcx>> for LocalDefId { |
There was a problem hiding this comment.
Hm, why do local def ids ever need to be decoded from metadata?
Or is this somehow used for decoding external DefIndexes, which are not actually local?
Then it's a hack, that needs at least a good comment explaining why we have to do this.
Ideally, we'd of course decode DefIndexes directly, even if they were decoded as LocalDefIds, but I agree that it may be annoying if they are e.g. use the same structure in compiler/rustc_metadata/src/rmeta/mod.rs.
There was a problem hiding this comment.
When LocalDefId is in the value of some table we need to decode it, we can access tables' keys with different types as we created infrastructure for this, but when the same situation happens in values there is more needed to be done to support it.
There was a problem hiding this comment.
Then it's a hack, that needs at least a good comment explaining why we have to do this.
Still needs a comment.
There was a problem hiding this comment.
Or is this somehow used for decoding external DefIndexes, which are not actually local?
They are local to the crate being decoded, so those def indices are external for the current crate but local to external, it makes some sense why we decoding local def ids.
|
(I'll finish the review later today, need to go now.) |
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 ofDefIndexencoding should always panic to prevent accidental remappings of non-localDefIndexes in #162809.So the plan is as follows:
LocalDefIdas a writing index in tables for metadata encoding, useLocalDefIdinstead ofDefIndexeverywhere, specialize encode/decode methods to correctly process it,encode_def_indexinEncodeContextwith panic, so we never remap and encode def index,LocalDefIdand read usingDefIndex(done in Allow using different index types when reading and writing to tables #163447).Blocked by #162900.
r? @petrochenkov