Skip to content

Conversation

bnjbvr
Copy link
Member

@bnjbvr bnjbvr commented Sep 3, 2025

The room notification modes will only change when the push rules have changed; otherwise, recomputing them is a no-op, and only wastes time.

Here's a distribution of the worst processing times under this method, as measured in multiverse with my personal account running (fresh, i.e. after an initial response) for a few minutes:

Before:

0.001358279s
0.019668889s
0.030104444s
0.030376296s
0.048609819s
0.049683619s
0.054289813s
0.063690030s

After:

0.000000385s
0.000000387s
0.000000410s
0.000000430s
0.000000438s
0.000000478s
0.000000487s
0.000000568s
0.010011267s

The new distribution makes sense: the push rules event comes only once, in the initial response, so they're processed only once. Then, there's no need to process it ever again (in my session I've never changed the processing times), so it's almost instantaneous the subsequent times.

Found thanks to #5612

…s have changed

The room notification modes will only change when the push rules have
changed; otherwise, recomputing them is a no-op, and only wastes time.

Here's a distribution of the worst processing times under this method,
as measured in multiverse with my personal account running (fresh, i.e.
after an initial response) for a few minutes:

Before:

0.001358279s
0.019668889s
0.030104444s
0.030376296s
0.048609819s
0.049683619s
0.054289813s
0.063690030s

After:

0.000000385s
0.000000387s
0.000000410s
0.000000430s
0.000000438s
0.000000478s
0.000000487s
0.000000568s
0.010011267s

The new distribution makes sense: the push rules event comes only once,
in the initial response, so they're processed only once. Then, there's
no need to process it ever again (in my session I've never changed the
processing times), so it's almost instantaneous the subsequent times.
@bnjbvr bnjbvr requested a review from a team as a code owner September 3, 2025 12:26
@bnjbvr bnjbvr requested review from andybalaam and removed request for a team September 3, 2025 12:26
Copy link

codecov bot commented Sep 3, 2025

Codecov Report

❌ Patch coverage is 76.92308% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.87%. Comparing base (7767ef6) to head (5ea16b9).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
crates/matrix-sdk/src/sliding_sync/client.rs 76.92% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5621      +/-   ##
==========================================
- Coverage   88.88%   88.87%   -0.01%     
==========================================
  Files         348      348              
  Lines       96451    96460       +9     
  Branches    96451    96460       +9     
==========================================
  Hits        85730    85730              
- Misses       6685     6693       +8     
- Partials     4036     4037       +1     

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

Copy link

codspeed-hq bot commented Sep 3, 2025

CodSpeed Performance Report

Merging #5621 will not alter performance

Comparing bnjbvr/process-room-notification-changes-only-when-push-rules-changed (5ea16b9) with main (bcabf1b)

Summary

✅ 49 untouched benchmarks

continue;
};

room.user_defined_notification_mode().await;
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: an extra perf improvement would be to create the NotificationSettings instance once (instead of once per call to user_defined_notification_mode()), and use it to update all the room notification modes at once, instead of creating the NotificationSettings (which loads the push rules from DB) once per room.

Also, I think it's still necessary to figure what are the newly joined rooms, and compute the user defined notification mode for those.

Not as simple as I imagined first, unfortunately.

Copy link
Member

@andybalaam andybalaam left a comment

Choose a reason for hiding this comment

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

Great, thanks. I am assuming this is already covered by adequate tests?

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