Encode trait impls using nested Tables #74807
Conversation
|
r? @lcnr (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
We now decode trait impls DefIds only as needed.
Aaron1011
force-pushed
the
feature/better-impl-encode
branch
from
0055248 to
119c788
Compare
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion |
|
⌛ Trying commit 119c788 with merge 15e932bee7e1345c8c86e007877d0dd0c5bc4a84... |
|
This is relevant to perf regressions in #74682. |
|
It would be great if @eddyb could review this. |
Yeah, my hope is to at least partially mitigate the impact of adding additional crates to the sysroot. |
|
☀️ Try build successful - checks-actions, checks-azure |
|
Queued 15e932bee7e1345c8c86e007877d0dd0c5bc4a84 with parent fa36f96, future comparison URL. |
|
Finished benchmarking try commit (15e932bee7e1345c8c86e007877d0dd0c5bc4a84): comparison url. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up. @bors rollup=never |
JohnCSimon
added
S-waiting-on-author
|
@Aaron1011 any updates on this? |
|
r? @eddyb |
petrochenkov
added
S-waiting-on-review
| @@ -286,6 +280,7 @@ define_tables! { | |||
| mir: Table<DefIndex, Lazy!(mir::Body<'tcx>)>, | |||
| promoted_mir: Table<DefIndex, Lazy!(IndexVec<mir::Promoted, mir::Body<'tcx>>)>, | |||
| unused_generic_params: Table<DefIndex, Lazy<FiniteBitSet<u64>>>, | |||
| trait_impls: Table<u32, Lazy!(Table<DefIndex, Lazy!([DefIndex])>)>, | |||
There was a problem hiding this comment.
We don't have sparse tables yet (I've been meaning to do that but haven't gotten around to it) so you should look at how much this change increases the size of libcore-*.rlib and libstd-*.rlib etc.
There was a problem hiding this comment.
You could maybe make this cheaper by removing the inner table and keeping a cache that's keyed on the u32 with a value of FxHashMap<DefIndex, Lazy<[DefIndex]>> for every crate.
Also, the u32, should that be CrateNum? That would be clearer and as a Table it shouldn't cause problems I don't think.
|
|
||
| let len = self.map_or(0, |lazy| lazy.meta); | ||
| let len: u32 = len.try_into().unwrap(); | ||
| impl<I: Idx, T: Encodable> FixedSizeEncoding for Option<Lazy<Table<I, Lazy<[T], usize>>, usize>> { |
There was a problem hiding this comment.
Lazy<[T], usize> is unnecessary specific here.
| @@ -93,24 +93,34 @@ impl<T: Encodable> FixedSizeEncoding for Option<Lazy<T>> { | |||
| } | |||
| } | |||
|
|
|||
| impl<T: Encodable> FixedSizeEncoding for Option<Lazy<[T]>> { | |||
| fixed_size_encoding_byte_len_and_defaults!(u32::BYTE_LEN * 2); | |||
| macro_rules! meta_body { | |||
There was a problem hiding this comment.
impl_fixed_size_encoding_for_usize_meta would be a better name, but also, have you tried this?
-impl<T: Encodable> FixedSizeEncoding for Option<Lazy<[T]>> {
+impl<T: ?Sized + Encodable + LazyMeta<Meta = usize>> FixedSizeEncoding for Option<Lazy<T>> {Although perhaps that is still considered overlapping, I'm not sure of the coherence rules around this.
There was a problem hiding this comment.
I've left some comments but I don't think there's a point in doing this if it's not a measurable perf win.
Also I keep forgetting about this, but perf.r-l.o should show stdlib crate sizes (and/or the size of the lib.rmeta file inside an rlib), in case something makes them increase substantially. cc @Mark-Simulacrum
(Or maybe we can have some other way to get that information? It can be obtained for arbitrary builds by downloading the rust-std component)
|
Oh, something else: as per #75008, it might make more sense to index the impls by the simplified type, not just the trait. Not sure what the best way to do that is, though. |
eddyb
removed
the
S-waiting-on-review
eddyb
added
the
S-waiting-on-author
|
Closing this for now - I may revisit it if I come up with a way to make it a noticable perf win. However, it may be that this never really matters in practice (e.g. most crates don't have very many trait impls, or access to them is usually all-or-nothing). |
We now decode trait impls DefIds only as needed.