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.

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

feat(base): Make Store::get_rooms wayyy faster #3552

Merged
merged 1 commit into from
Jun 13, 2024

Conversation

Hywan
Copy link
Member

@Hywan Hywan commented Jun 13, 2024

Store::get_rooms worked as follows: For each room ID, call

  • Take the read lock for rooms,
  • For each entry in rooms, take the room ID (the key), then call Store::get_room, which…
  • Take the read lock for rooms,
  • Based on the room ID (the key), read the room (the value) from rooms.

So calling get_rooms calls get_room for each room, which takes the lock every time.

This patch modifies that by fetching the values (the rooms) directly from rooms, without calling get_room.

In my benchmark, it took 1.2ms to read 10'000 rooms. Now it takes 542µs. Performance time has improved by -55.8%.


`Store::get_rooms` worked as follows: For each room ID, call

* Take the read lock for `rooms`,
* For each entry in `rooms`, take the room ID (the key), then call `Store::get_room`, which…
* Take the read lock for `rooms`,
* Based on the room ID (the key), read the room (the value) from `rooms`.

So calling `get_rooms` calls `get_room` for each room, which takes the lock every time.

This patch modifies that by fetching the values (the rooms) directly
from `rooms`, without calling `get_room`.

In my benchmark, it took 1.2ms to read 10'000 rooms. Now it takes 542µs.
Performance time has improved by -55.8%.
@Hywan Hywan requested a review from a team as a code owner June 13, 2024 14:03
@Hywan Hywan requested review from bnjbvr and removed request for a team June 13, 2024 14:03
Copy link
Member

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

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

Nice! (Would be nice to have a formal benchmark for this, that we can run with cargo-bench, but the change is so obvious and trivial…)

Can you also fix get_rooms_filtered, while you're at it, please? It does something similar.

Copy link

codecov bot commented Jun 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.82%. Comparing base (868e821) to head (15ab776).
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3552      +/-   ##
==========================================
+ Coverage   83.80%   83.82%   +0.01%     
==========================================
  Files         253      253              
  Lines       25854    25890      +36     
==========================================
+ Hits        21668    21703      +35     
- Misses       4186     4187       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Hywan
Copy link
Member Author

Hywan commented Jun 13, 2024

get_rooms_filtered is updated as part of #3068 (this commit is extracted from #3068) with a filter_map and so on, it's a bit different. Let's focus on get_rooms for the moment.

About the benchmark, I've sadly lost it! Never run git stash clear without being careful… I'll re-add it later with #3068 I believe.

@Hywan Hywan merged commit 9ccf94d into matrix-org:main Jun 13, 2024
38 checks passed
Hywan added a commit to Hywan/matrix-rust-sdk that referenced this pull request Jun 13, 2024
Just like in matrix-org#3552,
this patch updates `Store::rooms_filtered` to not call `Store::room` so
that it doesn't lock `rooms` for each item, thus making it way faster.
Hywan added a commit to Hywan/matrix-rust-sdk that referenced this pull request Jun 13, 2024
Just like in matrix-org#3552,
this patch updates `Store::rooms_filtered` to not call `Store::room` so
that it doesn't lock `rooms` for each item, thus making it way faster.
Hywan added a commit to Hywan/matrix-rust-sdk that referenced this pull request Jun 13, 2024
Just like in matrix-org#3552,
this patch updates `Store::rooms_filtered` to not call `Store::room` so
that it doesn't lock `rooms` for each item, thus making it way faster.
@Hywan
Copy link
Member Author

Hywan commented Jun 13, 2024

rooms_filtered is fixed in #3554

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.

2 participants