feat: faster VisibilityTracker::from_crate #404
Conversation
There was a problem hiding this comment.
This is a great optimization, nice find!
I'd love to make the new code a bit more maintenance-friendly before we merge it. As Rust evolves, we'll need to keep updating this code to cover new item kinds and it'll be important that we always understand how the code works and why. Otherwise, we'll write bugs and performance regressions, which will be frustrating to everyone.
| let visible_parent_ids = compute_parent_ids_for_public_items(crate_) | ||
| .into_iter() | ||
| .map(|(key, values)| { | ||
| // Ensure a consistent order, since queries can observe this order directly. |
There was a problem hiding this comment.
Let's try to preserve this comment, since I believe it's still relevant and accurate. Adding to my suggestion in the other comment.
src/visibility_tracker.rs
Outdated
| let iter = visible_parent_ids.iter_mut(); | ||
|
|
||
| // Sort and deduplicate parent ids | ||
| iter.for_each(|(_id, parent_ids)| { | ||
| parent_ids.sort_unstable_by_key(|id| id.0.as_str()); | ||
| parent_ids.dedup_by_key(|id| id.0.as_str()); | ||
| }); |
There was a problem hiding this comment.
A bit of personal preference, if you don't mind indulging me: I find it easier to understand iterator .for_each() when the base iterator is constructed inline instead of constructed separately and assigned to a variable.
| let iter = visible_parent_ids.iter_mut(); | |
| // Sort and deduplicate parent ids | |
| iter.for_each(|(_id, parent_ids)| { | |
| parent_ids.sort_unstable_by_key(|id| id.0.as_str()); | |
| parent_ids.dedup_by_key(|id| id.0.as_str()); | |
| }); | |
| // Sort and deduplicate parent ids. | |
| // This ensures a consistent order, since queries can observe the order directly. | |
| visible_parent_ids.iter_mut().for_each(|(_id, parent_ids)| { | |
| parent_ids.sort_unstable_by_key(|id| id.0.as_str()); | |
| parent_ids.dedup_by_key(|id| id.0.as_str()); | |
| }); |
src/visibility_tracker.rs
Outdated
| } | ||
|
|
||
| let next_parent_id = Some(&item.id); | ||
| // queue up removing this from the visited list |
There was a problem hiding this comment.
Just to match the style of the surrounding comments.
| // queue up removing this from the visited list | |
| // Queue up removing this from the visited list. |
There was a problem hiding this comment.
Fixed (by removing the refactor)
src/visibility_tracker.rs
Outdated
| stack.reserve(m.items.len()); | ||
| stack.extend(m.items.iter().filter_map(|id| { |
There was a problem hiding this comment.
It's interesting that this does .reserve() then .extend().
The iterator length of m.items.iter().filter_map() is known to be <= m.items.len(). Is this not something that the stack's .extend() will already account for?
There was a problem hiding this comment.
Fixed (by removing the refactor)
src/visibility_tracker.rs
Outdated
| stack.reserve(trait_.items.len()); | ||
| stack.extend(trait_.items.iter().filter_map(|id| { | ||
| crate_.index.get(id).map(|inner| Action::Process { | ||
| item: inner, | ||
| parent_id: next_parent_id, | ||
| }) | ||
| })); |
There was a problem hiding this comment.
Do you think it'd be feasible to make some kind of refactor to deduplicate this reserve() + extend() + crate_.index.get(id).map(Action::Process {...}) pattern?
This method was pretty complex to start with, and now includes even more complexity with a fair bit of extra repetition.
There was a problem hiding this comment.
Fixed (by removing the refactor)
src/visibility_tracker.rs
Outdated
| item_parents.insert(parent_id); | ||
| ) -> HashMap<&'a Id, Vec<&'a Id>> { | ||
| #[derive(Debug)] | ||
| enum Action<'a> { |
There was a problem hiding this comment.
It would be good to leave a doc comment explaining the purpose of this type.
It took me a non-trivial amount of time to figure out why it's present and how it works. I'm not confident that in 6 months you or I will still remember the details, so as a favor to our future selves, I'd like to err on the side of explaining in too much detail rather than too little.
There was a problem hiding this comment.
Fixed (by removing the refactor)
src/visibility_tracker.rs
Outdated
| let mut stack = vec![Action::Process { | ||
| item, | ||
| parent_id: None, | ||
| }]; |
There was a problem hiding this comment.
Similarly here, it would be good to leave an explanatory comment for what's happening: we're simulating recursion with a stack, the different actions mean A and B and correspond to equivalent operations X and Y in the recursive model. The stack-based implementation is faster because of reasons XYZ etc.
All this will be useful context for the next person who might try to update or optimize this code further.
There was a problem hiding this comment.
Fixed (by removing the refactor)
Lets push this PR back a bit. I'll split it into the manual dedup and the rewrite, because the more i look at the rewrite, the less i like it. |
|
Sounds good to me 👍 |
jalil-salame
force-pushed
the
improve-visibility-tracker
branch
from
019f146 to
5859ef7
Compare
We reduce the number of times `&Id`s get hashed and get a nice perf
boost thanks to that.
Numbers on my machine:
```console
$ cargo criterion --bench indexed_crate
IndexedCrate/new(aws-sdk-ec2)
time: [1.5595 s 1.5713 s 1.5827 s]
change: [-8.3651% -7.5024% -6.6525%] (p = 0.00 < 0.05)
Performance has improved.
```
jalil-salame
force-pushed
the
improve-visibility-tracker
branch
from
5859ef7 to
d658aba
Compare
* feat: faster VisibilityTracker::from_crate
We reduce the number of times `&Id`s get hashed and get a nice perf
boost thanks to that.
Numbers on my machine:
```console
$ cargo criterion --bench indexed_crate
IndexedCrate/new(aws-sdk-ec2)
time: [1.5595 s 1.5713 s 1.5827 s]
change: [-8.3651% -7.5024% -6.6525%] (p = 0.00 < 0.05)
Performance has improved.
```
* Update src/visibility_tracker.rs
---------
Co-authored-by: Predrag Gruevski <[email protected]>
* feat: faster VisibilityTracker::from_crate
We reduce the number of times `&Id`s get hashed and get a nice perf
boost thanks to that.
Numbers on my machine:
```console
$ cargo criterion --bench indexed_crate
IndexedCrate/new(aws-sdk-ec2)
time: [1.5595 s 1.5713 s 1.5827 s]
change: [-8.3651% -7.5024% -6.6525%] (p = 0.00 < 0.05)
Performance has improved.
```
* Update src/visibility_tracker.rs
---------
Co-authored-by: Predrag Gruevski <[email protected]>
* feat: faster VisibilityTracker::from_crate
We reduce the number of times `&Id`s get hashed and get a nice perf
boost thanks to that.
Numbers on my machine:
```console
$ cargo criterion --bench indexed_crate
IndexedCrate/new(aws-sdk-ec2)
time: [1.5595 s 1.5713 s 1.5827 s]
change: [-8.3651% -7.5024% -6.6525%] (p = 0.00 < 0.05)
Performance has improved.
```
* Update src/visibility_tracker.rs
---------
Co-authored-by: Predrag Gruevski <[email protected]>
* feat: faster VisibilityTracker::from_crate
We reduce the number of times `&Id`s get hashed and get a nice perf
boost thanks to that.
Numbers on my machine:
```console
$ cargo criterion --bench indexed_crate
IndexedCrate/new(aws-sdk-ec2)
time: [1.5595 s 1.5713 s 1.5827 s]
change: [-8.3651% -7.5024% -6.6525%] (p = 0.00 < 0.05)
Performance has improved.
```
* Update src/visibility_tracker.rs
---------
Co-authored-by: Predrag Gruevski <[email protected]>
* feat: faster VisibilityTracker::from_crate
We reduce the number of times `&Id`s get hashed and get a nice perf
boost thanks to that.
Numbers on my machine:
```console
$ cargo criterion --bench indexed_crate
IndexedCrate/new(aws-sdk-ec2)
time: [1.5595 s 1.5713 s 1.5827 s]
change: [-8.3651% -7.5024% -6.6525%] (p = 0.00 < 0.05)
Performance has improved.
```
* Update src/visibility_tracker.rs
---------
Co-authored-by: Jalil David Salamé Messina <[email protected]>
* feat: faster VisibilityTracker::from_crate
We reduce the number of times `&Id`s get hashed and get a nice perf
boost thanks to that.
Numbers on my machine:
```console
$ cargo criterion --bench indexed_crate
IndexedCrate/new(aws-sdk-ec2)
time: [1.5595 s 1.5713 s 1.5827 s]
change: [-8.3651% -7.5024% -6.6525%] (p = 0.00 < 0.05)
Performance has improved.
```
* Update src/visibility_tracker.rs
---------
Co-authored-by: Jalil David Salamé Messina <[email protected]>
* feat: faster VisibilityTracker::from_crate
We reduce the number of times `&Id`s get hashed and get a nice perf
boost thanks to that.
Numbers on my machine:
```console
$ cargo criterion --bench indexed_crate
IndexedCrate/new(aws-sdk-ec2)
time: [1.5595 s 1.5713 s 1.5827 s]
change: [-8.3651% -7.5024% -6.6525%] (p = 0.00 < 0.05)
Performance has improved.
```
* Update src/visibility_tracker.rs
---------
Co-authored-by: Jalil David Salamé Messina <[email protected]>
* feat: faster VisibilityTracker::from_crate
We reduce the number of times `&Id`s get hashed and get a nice perf
boost thanks to that.
Numbers on my machine:
```console
$ cargo criterion --bench indexed_crate
IndexedCrate/new(aws-sdk-ec2)
time: [1.5595 s 1.5713 s 1.5827 s]
change: [-8.3651% -7.5024% -6.6525%] (p = 0.00 < 0.05)
Performance has improved.
```
* Update src/visibility_tracker.rs
---------
Co-authored-by: Jalil David Salamé Messina <[email protected]>
We reduce the number of times
&Ids get hashed and get a nice perf boost thanks to that.Numbers on my machine:
Split off from #402 (perf numbers come from the commits there)