Skip to content

Commit

Permalink
feat: introduce internal IdRef type (#435) (#440)
Browse files Browse the repository at this point in the history
We use `&Id` in many places. As `Id` is defined as `struct Id(pub
String)` this is a ptr-to-ptr situation. By using `&IdRef(str)` we
remove that indirection, this increases memory usage as `&IdRef` is now
a "wide" ptr (ptr and length) instead of a "thin" ptr, but the perf win
is worth it:

```console
(32418dd)❯ cargo criterion --bench indexed_crate
                        time:   [1.3590 s 1.3599 s 1.3609 s]

(changes)❯ cargo criterion --bench indexed_crate
IndexedCrate/new(aws-sdk-ec2)
                        time:   [1.2456 s 1.2466 s 1.2478 s]
                        change: [-8.4413% -8.3275% -8.2222%] (p = 0.00 < 0.05)
                        Performance has improved.

(32418dd)❯ cargo criterion --bench indexed_crate --features rayon
IndexedCrate/new(aws-sdk-ec2)
                        time:   [571.63 ms 572.93 ms 574.23 ms]

(changes)❯ cargo criterion --bench indexed_crate --features rayon
IndexedCrate/new(aws-sdk-ec2)
                        time:   [501.28 ms 502.32 ms 503.34 ms]
                        change: [-12.596% -12.324% -12.053%] (p = 0.00 < 0.05)
                        Performance has improved.
```

The discussions on Zulip propose changing the `Id` definition in
`rustdoc-types` to `u64` or even `u32`, but while we wait for this
change to land (and for older `rustdoc` versions), this should be a nice
perf improvement.

The improvements are bigger when using `rayon`, because we spend more
time on `visibility_tracker` stuff (which is where the biggest
improvements are).

Co-authored-by: Jalil David Salamé Messina <60845989+jalil-salame@users.noreply.github.com>
  • Loading branch information
obi1kenobi and jalil-salame committed Sep 1, 2024
1 parent 8353776 commit d570922
Show file tree
Hide file tree
Showing 5 changed files with 94 additions and 60 deletions.
2 changes: 1 addition & 1 deletion src/adapter/edges.rs
Original file line number Diff line number Diff line change
Expand Up @@ -512,7 +512,7 @@ pub(super) fn resolve_trait_edge<'a, V: AsVertex<Vertex<'a>> + 'a>(

let trait_vertex = vertex.as_trait().expect("not a Trait vertex");
Box::new(trait_vertex.bounds.iter().filter_map(move |bound| {
if let TraitBound { trait_, .. } = bound {
if let TraitBound { trait_, .. } = &bound {
item_index
.get(&trait_.id)
.as_ref()
Expand Down
2 changes: 1 addition & 1 deletion src/adapter/optimizations/method_lookup.rs
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ fn resolve_methods_slow_path<'a>(
if let Some(trait_item) = trait_item {
if let ItemEnum::Trait(trait_item) = &trait_item.inner {
Box::new(trait_item.items.iter().filter(move |item_id| {
let next_item = &item_index.get(item_id);
let next_item = item_index.get(item_id.as_ref());
if let Some(name) = next_item.and_then(|x| x.name.as_deref()) {
method_names.contains(name)
} else {
Expand Down
34 changes: 17 additions & 17 deletions src/indexed_crate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,7 @@ impl<'a> IndexedCrate<'a> {
pub fn publicly_importable_names(&self, id: &'a Id) -> Vec<ImportablePath<'a>> {
if self.inner.index.contains_key(id) {
self.visibility_tracker
.collect_publicly_importable_names(id)
.collect_publicly_importable_names(id.as_ref())
} else {
Default::default()
}
Expand Down Expand Up @@ -593,23 +593,23 @@ mod tests {
assert!(indexed_crate
.visibility_tracker
.visible_parent_ids()
.contains_key(top_level_function));
.contains_key(top_level_function.as_ref()));
assert!(indexed_crate
.visibility_tracker
.visible_parent_ids()
.contains_key(method));
.contains_key(method.as_ref()));
assert!(indexed_crate
.visibility_tracker
.visible_parent_ids()
.contains_key(associated_fn));
.contains_key(associated_fn.as_ref()));
assert!(indexed_crate
.visibility_tracker
.visible_parent_ids()
.contains_key(field));
.contains_key(field.as_ref()));
assert!(indexed_crate
.visibility_tracker
.visible_parent_ids()
.contains_key(const_item));
.contains_key(const_item.as_ref()));

// But only `top_level_function` is importable.
assert_eq!(
Expand Down Expand Up @@ -655,23 +655,23 @@ mod tests {
assert!(indexed_crate
.visibility_tracker
.visible_parent_ids()
.contains_key(top_level_function));
.contains_key(top_level_function.as_ref()));
assert!(indexed_crate
.visibility_tracker
.visible_parent_ids()
.contains_key(variant));
.contains_key(variant.as_ref()));
assert!(indexed_crate
.visibility_tracker
.visible_parent_ids()
.contains_key(method));
.contains_key(method.as_ref()));
assert!(indexed_crate
.visibility_tracker
.visible_parent_ids()
.contains_key(associated_fn));
.contains_key(associated_fn.as_ref()));
assert!(indexed_crate
.visibility_tracker
.visible_parent_ids()
.contains_key(const_item));
.contains_key(const_item.as_ref()));

// But only `top_level_function` and `Foo::variant` is importable.
assert_eq!(
Expand Down Expand Up @@ -721,27 +721,27 @@ mod tests {
assert!(indexed_crate
.visibility_tracker
.visible_parent_ids()
.contains_key(top_level_function));
.contains_key(top_level_function.as_ref()));
assert!(indexed_crate
.visibility_tracker
.visible_parent_ids()
.contains_key(method));
.contains_key(method.as_ref()));
assert!(indexed_crate
.visibility_tracker
.visible_parent_ids()
.contains_key(associated_fn));
.contains_key(associated_fn.as_ref()));
assert!(indexed_crate
.visibility_tracker
.visible_parent_ids()
.contains_key(left_field));
.contains_key(left_field.as_ref()));
assert!(indexed_crate
.visibility_tracker
.visible_parent_ids()
.contains_key(right_field));
.contains_key(right_field.as_ref()));
assert!(indexed_crate
.visibility_tracker
.visible_parent_ids()
.contains_key(const_item));
.contains_key(const_item.as_ref()));

// But only `top_level_function` is importable.
assert_eq!(
Expand Down
2 changes: 1 addition & 1 deletion src/sealed_trait.rs
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,7 @@ fn blanket_type_might_cover_types_in_downstream_crate(blanket_type: &rustdoc_typ
true
}
rustdoc_types::Type::BorrowedRef { type_, .. } => {
// Blanket implementatio over a reference, like `&T`.
// Blanket implementation over a reference, like `&T`.
// It matches if the underlying type beheath the reference matches.
blanket_type_might_cover_types_in_downstream_crate(type_)
}
Expand Down
Loading

0 comments on commit d570922

Please sign in to comment.