Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor vtable encoding and optimize it for the case of multiple marker traits #113856

Merged
merged 9 commits into from
Jul 20, 2023
Merged

Refactor vtable encoding and optimize it for the case of multiple marker traits #113856

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
eabd306
Document `PredicateSet::insert`
WaffleLapkin Jul 19, 2023
348f26e
Use `?` in `prepare_vtable_segments`
WaffleLapkin Jul 19, 2023
f8f5d7a
Replace `if let` with `unwrap` in `prepare_vtable_segments`
WaffleLapkin Jul 19, 2023
364fc44
Refactor 'diving_in loop internals in `prepare_vtable_segments`
WaffleLapkin Jul 19, 2023
d567f0f
Slightly refactor 'exiting_out loop in `prepare_vtable_segments`
WaffleLapkin Jul 19, 2023
d87db8e
Simplify last `prepare_vtable_segments` loop even more
WaffleLapkin Jul 19, 2023
f33936c
Fix comment
WaffleLapkin Jul 19, 2023
b5d1228
Add a (failing test) for issue `113840`
WaffleLapkin Jul 19, 2023
1f02c75
Don't emit useless vptrs for marker traits
WaffleLapkin Jul 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 21 additions & 4 deletions compiler/rustc_trait_selection/src/traits/vtable.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,16 +147,21 @@ fn prepare_vtable_segments_inner<'tcx, T>(
}
}

// Other than the left-most path, vptr should be emitted for each trait.
emit_vptr_on_new_entry = true;

// emit innermost item, move to next sibling and stop there if possible, otherwise jump to outer level.
while let Some((inner_most_trait_ref, emit_vptr, mut siblings)) = stack.pop() {
segment_visitor(VtblSegment::TraitOwnEntries {
trait_ref: inner_most_trait_ref,
emit_vptr,
})?;

// If we've emitted (fed to `segment_visitor`) a trait that has methods present in the vtable,
// we'll need to emit vptrs from now on.
if !emit_vptr_on_new_entry
&& has_own_existential_vtable_entries(tcx, inner_most_trait_ref.def_id())
{
emit_vptr_on_new_entry = true;
}

if let Some(next_inner_most_trait_ref) =
siblings.find(|&sibling| visited.insert(sibling.to_predicate(tcx)))
{
Expand Down Expand Up @@ -196,11 +201,23 @@ fn dump_vtable_entries<'tcx>(
});
}

fn has_own_existential_vtable_entries(tcx: TyCtxt<'_>, trait_def_id: DefId) -> bool {
own_existential_vtable_entries_iter(tcx, trait_def_id).next().is_some()
}

fn own_existential_vtable_entries(tcx: TyCtxt<'_>, trait_def_id: DefId) -> &[DefId] {
tcx.arena.alloc_from_iter(own_existential_vtable_entries_iter(tcx, trait_def_id))
}

fn own_existential_vtable_entries_iter(
tcx: TyCtxt<'_>,
trait_def_id: DefId,
) -> impl Iterator<Item = DefId> + '_ {
let trait_methods = tcx
.associated_items(trait_def_id)
.in_definition_order()
.filter(|item| item.kind == ty::AssocKind::Fn);

// Now list each method's DefId (for within its trait).
let own_entries = trait_methods.filter_map(move |&trait_method| {
debug!("own_existential_vtable_entry: trait_method={:?}", trait_method);
Expand All @@ -215,7 +232,7 @@ fn own_existential_vtable_entries(tcx: TyCtxt<'_>, trait_def_id: DefId) -> &[Def
Some(def_id)
});

tcx.arena.alloc_from_iter(own_entries.into_iter())
own_entries
}

/// Given a trait `trait_ref`, iterates the vtable entries
Expand Down
5 changes: 3 additions & 2 deletions tests/ui/traits/object/print_vtable_sizes.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,9 @@ trait C {
fn x() {} // not object safe, shouldn't be reported
}

// This ideally should not have any upcasting cost,
// but currently does due to a bug
// This does not have any upcasting cost,
// because both `Send` and `Sync` are traits
// with no methods
trait D: Send + Sync + help::MarkerWithSuper {}

// This can't have no cost without reordering,
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/traits/object/print_vtable_sizes.stdout
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
print-vtable-sizes { "crate_name": "<UNKNOWN_CRATE>", "trait_name": "D", "entries": "7", "entries_ignoring_upcasting": "4", "entries_for_upcasting": "3", "upcasting_cost_percent": "75" }
print-vtable-sizes { "crate_name": "<UNKNOWN_CRATE>", "trait_name": "E", "entries": "6", "entries_ignoring_upcasting": "4", "entries_for_upcasting": "2", "upcasting_cost_percent": "50" }
print-vtable-sizes { "crate_name": "<UNKNOWN_CRATE>", "trait_name": "G", "entries": "14", "entries_ignoring_upcasting": "11", "entries_for_upcasting": "3", "upcasting_cost_percent": "27.27272727272727" }
print-vtable-sizes { "crate_name": "<UNKNOWN_CRATE>", "trait_name": "A", "entries": "6", "entries_ignoring_upcasting": "5", "entries_for_upcasting": "1", "upcasting_cost_percent": "20" }
print-vtable-sizes { "crate_name": "<UNKNOWN_CRATE>", "trait_name": "B", "entries": "4", "entries_ignoring_upcasting": "4", "entries_for_upcasting": "0", "upcasting_cost_percent": "0" }
print-vtable-sizes { "crate_name": "<UNKNOWN_CRATE>", "trait_name": "D", "entries": "4", "entries_ignoring_upcasting": "4", "entries_for_upcasting": "0", "upcasting_cost_percent": "0" }
print-vtable-sizes { "crate_name": "<UNKNOWN_CRATE>", "trait_name": "F", "entries": "6", "entries_ignoring_upcasting": "6", "entries_for_upcasting": "0", "upcasting_cost_percent": "0" }
print-vtable-sizes { "crate_name": "<UNKNOWN_CRATE>", "trait_name": "_::S", "entries": "3", "entries_ignoring_upcasting": "3", "entries_for_upcasting": "0", "upcasting_cost_percent": "0" }
print-vtable-sizes { "crate_name": "<UNKNOWN_CRATE>", "trait_name": "_::S", "entries": "3", "entries_ignoring_upcasting": "3", "entries_for_upcasting": "0", "upcasting_cost_percent": "0" }
Expand Down
6 changes: 0 additions & 6 deletions tests/ui/traits/vtable/multiple-markers.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,7 @@ error: vtable entries for `<S as A>`: [
MetadataDropInPlace,
MetadataSize,
MetadataAlign,
TraitVPtr(<S as M1>),
TraitVPtr(<S as M2>),
Method(<S as T>::method),
TraitVPtr(<S as T>),
]
--> $DIR/multiple-markers.rs:21:1
|
Expand All @@ -16,9 +13,7 @@ error: vtable entries for `<S as B>`: [
MetadataDropInPlace,
MetadataSize,
MetadataAlign,
TraitVPtr(<S as M1>),
Method(<S as T>::method),
TraitVPtr(<S as T>),
TraitVPtr(<S as M2>),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is TraitVPtr needed for M2?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The short answer: it is not needed. The longer answer is that we currently keep the order of all things from the traits, so since M2 comes after T, it can't be inlined there. I've filled #114942 to track this.

]
--> $DIR/multiple-markers.rs:24:1
Expand All @@ -31,7 +26,6 @@ error: vtable entries for `<S as C>`: [
MetadataSize,
MetadataAlign,
Method(<S as T>::method),
TraitVPtr(<S as T>),
TraitVPtr(<S as M1>),
TraitVPtr(<S as M2>),
]
Expand Down