Skip duplicate stable crate ID encoding into metadata #119238
Conversation
|
r? @wesleywiser (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
… r=<try> Specialize DefPathHash table to skip encoding crate IDs The current implementation is ad-hoc and likely should be replaced with a non-table based approach (i.e., fully pulling out DefPathHash from the rmeta table infrastructure, of which we use ~none now), but this was an easy way to get an initial PR out. The main pending question is whether the assumption made here that there is exactly one shared prefix accurate? If not, is it right that the number should be typically small? (If so a deduplication scheme of which this is a special case almost certainly makes sense). We encode a lot of these (1000s) so the savings of 8 bytes/hash add up quickly. Opening this PR to get opinions more on the general idea and to run perf on whether the underlying impl will perform OK.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-author
Mark-Simulacrum
force-pushed
the
def-hash-efficiency
branch
from
5b3116c to
7ad670d
Compare
|
@bors try @rust-timer queue |
… r=<try> Specialize DefPathHash table to skip encoding crate IDs The current implementation is ad-hoc and likely should be replaced with a non-table based approach (i.e., fully pulling out DefPathHash from the rmeta table infrastructure, of which we use ~none now), but this was an easy way to get an initial PR out. The main pending question is whether the assumption made here that there is exactly one shared prefix accurate? If not, is it right that the number should be typically small? (If so a deduplication scheme of which this is a special case almost certainly makes sense). We encode a lot of these (1000s) so the savings of 8 bytes/hash add up quickly. Opening this PR to get opinions more on the general idea and to run perf on whether the underlying impl will perform OK.
Mark-Simulacrum
changed the title
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
| // N.B. this means that we can't distinguish between non-present items and a present but zero | ||
| // local hash item. In practice the compiler shouldn't care about non-present items in a foreign | ||
| // crate, so this should be OK. If we do start to care we should most likely adjust our hashing | ||
| // to reserve a bit (e.g., NonZeroU64). |
There was a problem hiding this comment.
On the contrary: having a DefIndex with no associated DefPathHash should be a bug. So it's not an issue if we cannot distinguish the default: it must not appear.
There was a problem hiding this comment.
Hm, I'm not 100% sure that is upheld today (an earlier version of this PR hit some asserts in encoding that looked like zeros in the crate hash, which I assumed to mean that we had defaults present somewhere), see #119238 (comment) and this assert: rust-lang-ci@5b3116c#diff-6c291b96ad60b2cdaad44471b95c96d98c10d2eb9ed8dab44fe9e21ef7f1bd5eR508
Updated the comment for now and didn't dig deeper on re-introducing asserts etc.
|
Finished benchmarking commit (86e86bf): comparison URL. Overall result: no relevant changes - no action neededBenchmarking 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 Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeResultsThis 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.
Bootstrap: 669.674s -> 670.033s (0.05%) |
rustbot
removed
the
S-waiting-on-perf
Instead, we store just the local crate hash as a bare u64. On decoding, we recombine it with the crate's stable crate ID stored separately in metadata. The end result is that we save ~8 bytes/DefIndex in metadata size. One key detail here is that we no longer distinguish in encoded metadata between present and non-present DefPathHashes. It used to be highly likely we could distinguish as we used DefPathHash::default(), an all-zero representation. However in theory even that is fallible as nothing strictly prevents the StableCrateId from being zero.
Mark-Simulacrum
force-pushed
the
def-hash-efficiency
branch
from
7ad670d to
6630d69
Compare
Remove metadata decoding DefPathHash cache My expectation is that this cache is largely useless. Decoding a DefPathHash from metadata is essentially a pair of memory loads - there's no heavyweight processing involved. Caching it behind a HashMap just adds extra cost and incurs hashing overheads for the indices. Based on rust-lang#119238.
Mark-Simulacrum
removed
the
S-waiting-on-author
Mark-Simulacrum
added
the
S-waiting-on-review
|
Great! |
bors
added
S-waiting-on-bors
Only store StableCrateId once in DefPathTable. rust-lang#119238 made me think of this. cc `@Mark-Simulacrum`
… r=cjgillot Skip duplicate stable crate ID encoding into metadata Instead, we store just the local crate hash as a bare u64. On decoding, we recombine it with the crate's stable crate ID stored separately in metadata. The end result is that we save ~8 bytes/DefIndex in metadata size. One key detail here is that we no longer distinguish in encoded metadata between present and non-present DefPathHashes. It used to be highly likely we could distinguish as we used DefPathHash::default(), an all-zero representation. However in theory even that is fallible as nothing strictly prevents the StableCrateId from being zero. In review it was pointed out that we should never have a missing hash for a DefIndex anyway, so this shouldn't matter.
|
💥 Test timed out |
bors
added
S-waiting-on-review
|
@bors retry |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (cf64273): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)ResultsThis 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.
CyclesResultsThis 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.
Binary sizeResultsThis 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.
Bootstrap: 671.528s -> 670.535s (-0.15%) |
…llot Remove metadata decoding DefPathHash cache My expectation is that this cache is largely useless. Decoding a DefPathHash from metadata is essentially a pair of memory loads - there's no heavyweight processing involved. Caching it behind a HashMap just adds extra cost and incurs hashing overheads for the indices. Based on rust-lang#119238.
…llot Remove metadata decoding DefPathHash cache My expectation is that this cache is largely useless. Decoding a DefPathHash from metadata is essentially a pair of memory loads - there's no heavyweight processing involved. Caching it behind a HashMap just adds extra cost and incurs hashing overheads for the indices. Based on rust-lang#119238.
…ulacrum Only store StableCrateId once in DefPathTable. rust-lang#119238 made me think of this. cc `@Mark-Simulacrum`
Only store StableCrateId once in DefPathTable. rust-lang/rust#119238 made me think of this. cc `@Mark-Simulacrum`
Instead, we store just the local crate hash as a bare u64. On decoding,
we recombine it with the crate's stable crate ID stored separately in
metadata. The end result is that we save ~8 bytes/DefIndex in metadata
size.
One key detail here is that we no longer distinguish in encoded metadata
between present and non-present DefPathHashes. It used to be highly
likely we could distinguish as we used DefPathHash::default(), an
all-zero representation. However in theory even that is fallible as
nothing strictly prevents the StableCrateId from being zero. In review it
was pointed out that we should never have a missing hash for a DefIndex anyway,
so this shouldn't matter.