Skip to content

Conversation

@pickfire
Copy link
Contributor

Routes in path seemed to work with linear memory instead of needing the extra hash and tracking of prev_route_id, a Vec should work just fine while having the benefits of cache locality.

Motivation

Vec seemed to be able to fulfill routes without needing HashMap, as well as better cache locality.

Solution

Change PathRouter to use Vec.

Routes in path seemed to work with linear memory instead of needing the
extra hash and tracking of prev_route_id, a Vec should work just fine
while having the benefits of cache locality.
Copy link
Collaborator

@mladedav mladedav left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good as it might simplify things though I would be surprised if cache locality played any measurable role here as HashMaps are also backed by a single allocation and I would be surprised if the interleaved u32s made much difference. Plus it's iterated over in just a few scenarios.

@yanns
Copy link
Collaborator

yanns commented Nov 20, 2025

Under which scenarios would we iterate over all elements instead of using the optimized cache access?

@mladedav
Copy link
Collaborator

I think we iterate just when we merge/nest routers. I'm not sure what do you mean by optimized cache access.

Disrgarding any cache locality arguments, I would still merge this because it cleans up the code by not having to track prev_route_id explicitly.

This is on me that I did not do so earlier but I'm happy to do that now unless you see there some issues (or prefer the original for some reason).

@yanns
Copy link
Collaborator

yanns commented Nov 21, 2025

I'm not sure what do you mean by optimized cache access.

Usually, one uses a hashmap to avoid a complete iteration, and to get "optimized" access by key.

I don't see any issues with it. I also think it's a nice change. You can go forward with it. ❤️

@mladedav mladedav merged commit 26367b9 into tokio-rs:main Nov 21, 2025
18 checks passed
@mladedav
Copy link
Collaborator

For merging & nesting there is really no way around the full iteration, we have to empty one vector/map and insert those into the other, there's no iteration to find a given value if I'm not mistaken if that's the iteration you mean. When looking up a given route, we always know the route ID so now instead of hashing it and using that for the lookup, we index by that directly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants