Skip to content

fix(sdk): Introduce SubscribersHandle and SubscriberHandle#6684

Merged
Hywan merged 6 commits into
matrix-org:mainfrom
Hywan:fix-sdk-event-cache-shrink-all-caches
Jun 25, 2026
Merged

fix(sdk): Introduce SubscribersHandle and SubscriberHandle#6684
Hywan merged 6 commits into
matrix-org:mainfrom
Hywan:fix-sdk-event-cache-shrink-all-caches

Conversation

@Hywan

@Hywan Hywan commented Jun 23, 2026

Copy link
Copy Markdown
Member

This PR is twofold.

When a cache (from the Event Cache) has no more subscribers, it auto-shrinks to its last chunk to save memory. That's great! The way it counts/tracks the number of subscribers isn't efficient though. It uses an Arc<AtomicU32>. Arc in itself is already an atomic counter. We can use Arc<()> directly, saving the cost of more atomic operations.

The first main patch introduces SubscribersHandle that is owned by the cache state (like RoomEventCacheState). When a subscriber is created, SubscribersHandle::new_subscriber_handle (note the singular form) is called, and produces a SubscriberHandle (still singular form).

A SubscriberHandle is a thin type around Weak<()>. The state has an Arc, the subscribers have a Weak. The number of subscribers is calculated by (Arc|Weak)::weak_count. That way, no need to fetch_add or load to calculate the count.

The second main patch removes the sending of the VectorDiffs updates when the cache is shrunk. The documentation said:

Hey, fun stuff: we shrunk the linked chunk, so there shouldn't be any subscribers, right? RIGHT? Especially because the state is guarded behind a lock.

However, better safe than sorry, and it's cheap to send an update here, so let's do it!

I think that's an error. Two situations here:

  1. No race, no subscribers have been registered, so it's safe to do nothing,
  2. A race, a subscriber has been created meanwhile, we must not send the diff to it, otherwise it can create an invalid state.

Note that a race shouldn't be possible as we have acquired an exclusive access to the state, ensuring no subscriber can be created, so technically the diff should be sent to nowhere, but it's really important to not send it :-].



  • I've documented the public API changes in the appropriate changelog files (see Writing changelog entries).
  • This PR was made with the help of AI.

Signed-off-by:

Hywan added 2 commits June 23, 2026 18:31
Even if `AutoSkrinkChannelPayload` is an alias to `OwnedRoomId`, it's
still better to use the correct type here in case it changes in the
future.
@Hywan Hywan marked this pull request as ready for review June 23, 2026 16:40
@Hywan Hywan requested a review from a team as a code owner June 23, 2026 16:40
@Hywan Hywan requested review from poljar and removed request for a team June 23, 2026 16:40
@Hywan Hywan marked this pull request as draft June 23, 2026 16:47
Hywan added 3 commits June 23, 2026 18:48
When a cache (from the Event Cache) has no more subscribers, it
auto-shrinks to its last chunk to save memory. That's great! The way it
counts/tracks the number of subscribers isn't efficient though. It uses
an `Arc<AtomicU32>`. `Arc` in itself is already an atomic counter. We
can use `Arc<()>` directly, saving the cost of more atomic operations.

This patch introduces `SubscribersHandle` that is owned by the cache
state (like `RoomEventCacheState`). When a subscriber is created,
`SubscribersHandle::new_subscriber_handle` (note the singular form) is
called, and produces a `SubscriberHandle` (still singular form).

A `SubscriberHandle` is a thin type around `Weak<()>`. The state has
an `Arc`, the subscribers have a `Weak`. The number of subscribers is
calculated by `(Arc|Weak)::weak_count`. That way, no need to `fetch_add`
or `load` to calculate the count.

This patch adds a test to ensure the new types are correct and work
as expected.
This patch removes the sending of the `VectorDiff`s updates. The
documentation said:

> Hey, fun stuff: we shrunk the linked chunk, so there shouldn't be any
> subscribers, right? RIGHT? Especially because the state is guarded
> behind a lock.
>
> However, better safe than sorry, and it's cheap to send an update
> here, so let's do it!

I think that's an error. Two situations here:

1. No race, no subscribers have been registered, so it's safe
   to do nothing,
2. A race, a subscriber has been created meanwhile, we **must
   not** send the diff to it, otherwise it can create an
   invalid state.
@codecov

codecov Bot commented Jun 23, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.33333% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 89.87%. Comparing base (e8673e4) to head (d7a6535).
⚠️ Report is 9 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...ates/matrix-sdk/src/event_cache/caches/room/mod.rs 88.88% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6684      +/-   ##
==========================================
- Coverage   89.88%   89.87%   -0.02%     
==========================================
  Files         396      396              
  Lines      110264   110307      +43     
  Branches   110264   110307      +43     
==========================================
+ Hits        99115    99142      +27     
- Misses       7385     7386       +1     
- Partials     3764     3779      +15     

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

@Hywan Hywan marked this pull request as ready for review June 23, 2026 16:49
@Hywan Hywan force-pushed the fix-sdk-event-cache-shrink-all-caches branch from dd316bf to 4347843 Compare June 23, 2026 16:49
@codspeed-hq

codspeed-hq Bot commented Jun 23, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 50 untouched benchmarks


Comparing Hywan:fix-sdk-event-cache-shrink-all-caches (d7a6535) with main (4bd5e81)

Open in CodSpeed

@poljar poljar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this is mostly fine.

I wonder if we couldn't just use the strong_count() on the auto_shrink_sender() to get the subscriber count.

Also, how come no tests needed to be modified for the last patch? 🙈

@Hywan

Hywan commented Jun 25, 2026

Copy link
Copy Markdown
Member Author

I wonder if we couldn't just use the strong_count() on the auto_shrink_sender() to get the subscriber count.

Actually, we need to count the number of subscribers, so the weak_count(). The strong_count() would always give 1, as it lives in RoomEventCacheState only.

Also, how come no tests needed to be modified for the last patch? 🙈

Because I struggle to see how to test it. The auto-shrink happens if and only if no more subscriber exists. How to test nothing is sent if we can't receive it…? Hmm, now I'm giving it a second thought, it might be possible if we write the test at the correct place as a unit test, so that we can have access to the private fields. Let me try.

@Hywan Hywan enabled auto-merge (rebase) June 25, 2026 13:40
@Hywan Hywan merged commit fd1a69e into matrix-org:main Jun 25, 2026
53 checks passed
@Hywan Hywan mentioned this pull request Jun 26, 2026
21 tasks
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