Remove cached wrap on `_get_joined_users_from_context` method #13569

Fizzadar · 2022-08-19T12:45:02Z

The method doesn't actually do any data fetching and the method that does, _get_joined_profile_from_event_id, has it's own cache. Spotted that, at least on our install, the cache is effectively useless:

This does remove an optimisation in using the previous state group, but if the above stats are true elsewhere this provides little benefit anyway.

Signed off by Nick @ Beeper (@Fizzadar).

Pull Request Checklist

Pull request is based on the develop branch
Pull request includes a changelog file.
Pull request includes a sign off
Code style is correct
(run the linters)

The method doesn't actually do any data fetching and the method that does, `_get_joined_profile_from_event_id`, has it's own cache.

If the cache hits are indeed next to zero for this method, the context optimisation provides little benefit and thus can be removed.

richvdh · 2022-08-24T09:15:33Z

synapse/storage/databases/main/roommember.py

        state_group: Union[object, int],
        current_state_ids: StateMap[str],
-        event: Optional[EventBase] = None,
-        context: Optional["_StateCacheEntry"] = None,


note to self/for posterity: context has always been not-None since #8070 removed the only call that doesn't provide it (https://github.com/matrix-org/synapse/pull/8070/files#diff-54aaace5758538a90a9ed3a020eb9b260139269ee33c10ee8b2caf850c8a3e1dL290)

also: #13267 changed things so that it was always a _StateCacheEntry. Previously (since time immemorial) it could have been either an EventContext or a _StateCacheEntry. Both classes have properties like delta_ids and prev_group, so this worked, though it's a miracle we never broke it.

richvdh

wow, this was a mess. Good spot, thanks!

richvdh · 2022-08-24T09:25:19Z

synapse/storage/databases/main/roommember.py

        # We don't use `state_group`, it's there so that we can cache based
        # on it. However, it's important that it's never None, since two current_states


we no longer need state_group at all.

Removed alongwith the state entry arg in: 5f99fe8

richvdh · 2022-08-24T09:27:04Z

synapse/storage/databases/main/roommember.py


-    @cached(num_args=2, iterable=True, max_entries=100000)
    async def _get_joined_users_from_context(


given that there is no longer any context, this is an increasingly silly name.

I think it is now only called from get_joined_users_from_state - can we just inline it?

Much better! 63967da

richvdh · 2022-08-24T09:29:58Z

synapse/storage/databases/main/roommember.py

        state_group: Union[object, int],
        current_state_ids: StateMap[str],
-        event: Optional[EventBase] = None,


context: this also seems to have been unused since #13267.

richvdh · 2022-08-24T09:32:20Z

changelog.d/13569.misc

@@ -0,0 +1 @@
+Remove cache on `_get_joined_users_from_context` calls that are already cached elsewhere. Contributed by Nick @ Beeper (@fizzadar).


I think we should make this more explicit, somehow, since it means that any local configuration of the cache size is redundant, so admins may wish to update their config.

Like, rephrase it as "Remove redundant _get_joined_users_from_context cache", and make it a .removal rather than a .misc?

…m_state` methods

# Conflicts: # synapse/state/__init__.py # synapse/storage/databases/main/roommember.py

Fizzadar · 2022-08-25T08:55:03Z

Should be good to go @richvdh - bit of a hairy merge due to my other PR 🤦

richvdh · 2022-08-25T14:05:12Z

synapse/storage/databases/main/roommember.py

+    async def get_joined_users_from_state(
+        self, room_id: str, state: StateMap[str]
+    ) -> Dict[str, ProfileInfo]:


It looks like the merge has gone awry here: we're now reverting get_joined_user_ids_from_state back to get_joined_users_from_state (cf #13573)

Another 🤦 for me today - 6404481 (+ 4e62c46).

richvdh

looks good apart from some nit-picking about comments

richvdh · 2022-08-25T16:13:33Z

synapse/storage/databases/main/roommember.py

+        For a given set of state IDs, get a map of user ID to profile information
+        for users in the room.


It doesn't return profile information any more!

richvdh · 2022-08-25T16:20:46Z

synapse/storage/databases/main/roommember.py

+        This method doesn't do any fetching of data itself and instead checks various
+        caches for the relevant data before passing any remaining missing event IDs to
+        `_get_joined_profiles_from_event_ids` which does the actual data fetching.


I'm somewhat confused by what this text means. Possibly not helped by the fact that _get_joined_profiles_from_event_ids no longer exists, but in any case, I think it's misleading:

This method doesn't do any fetching of data itself

... except that it does. Admittedly, under the hood, it uses _get_user_ids_from_membership_event_ids to do that work, but that's an implementation detail. It's like saying "This method doesn't do any fetching of data itself, instead it uses simple_select_many to do the lookup". It might be true in some sense, but it's just not helpful information.

I think what you mean is something like "This method checks the local event cache, before calling _get_user_ids_from_membership_event_ids for any uncached events." ?

richvdh

thanks!

@Vetchu

Synapse 1.67.0 (2022-09-13) =========================== This release removes using the deprecated direct TCP replication configuration for workers. Server admins should use Redis instead. See the [upgrade notes](https://matrix-org.github.io/synapse/v1.67/upgrade.html#upgrading-to-v1670). The minimum version of `poetry` supported for managing source checkouts is now 1.2.0. **Notice:** from the next major release (1.68.0) installing Synapse from a source checkout will require a recent Rust compiler. Those using packages or `pip install matrix-synapse` will not be affected. See the [upgrade notes](https://matrix-org.github.io/synapse/v1.67/upgrade.html#upgrading-to-v1670). **Notice:** from the next major release (1.68.0), running Synapse with a SQLite database will require SQLite version 3.27.0 or higher. (The [current minimum version is SQLite 3.22.0](https://github.com/matrix-org/synapse/blob/release-v1.67/synapse/storage/engines/sqlite.py#L69-L78).) See [matrix-org#12983](matrix-org#12983) and the [upgrade notes](https://matrix-org.github.io/synapse/v1.67/upgrade.html#upgrading-to-v1670) for more details. No significant changes since 1.67.0rc1. Synapse 1.67.0rc1 (2022-09-06) ============================== Features -------- - Support setting the registration shared secret in a file, via a new `registration_shared_secret_path` configuration option. ([\matrix-org#13614](matrix-org#13614)) - Change the default startup behaviour so that any missing "additional" configuration files (signing key, etc) are generated automatically. ([\matrix-org#13615](matrix-org#13615)) - Improve performance of sending messages in rooms with thousands of local users. ([\matrix-org#13634](matrix-org#13634)) Bugfixes -------- - Fix a bug introduced in Synapse 1.13 where the [List Rooms admin API](https://matrix-org.github.io/synapse/develop/admin_api/rooms.html#list-room-api) would return integers instead of booleans for the `federatable` and `public` fields when using a Sqlite database. ([\matrix-org#13509](matrix-org#13509)) - Fix bug that user cannot `/forget` rooms after the last member has left the room. ([\matrix-org#13546](matrix-org#13546)) - Faster Room Joins: fix `/make_knock` blocking indefinitely when the room in question is a partial-stated room. ([\matrix-org#13583](matrix-org#13583)) - Fix loading the current stream position behind the actual position. ([\matrix-org#13585](matrix-org#13585)) - Fix a longstanding bug in `register_new_matrix_user` which meant it was always necessary to explicitly give a server URL. ([\matrix-org#13616](matrix-org#13616)) - Fix the running of [MSC1763](matrix-org/matrix-spec-proposals#1763) retention purge_jobs in deployments with background jobs running on a worker by forcing them back onto the main worker. Contributed by Brad @ Beeper. ([\matrix-org#13632](matrix-org#13632)) - Fix a long-standing bug that downloaded media for URL previews was not deleted while database background updates were running. ([\matrix-org#13657](matrix-org#13657)) - Fix [MSC3030](matrix-org/matrix-spec-proposals#3030) `/timestamp_to_event` endpoint to return the correct next event when the events have the same timestamp. ([\matrix-org#13658](matrix-org#13658)) - Fix bug where we wedge media plugins if clients disconnect early. Introduced in v1.22.0. ([\matrix-org#13660](matrix-org#13660)) - Fix a long-standing bug which meant that keys for unwhitelisted servers were not returned by `/_matrix/key/v2/query`. ([\matrix-org#13683](matrix-org#13683)) - Fix a bug introduced in Synapse v1.20.0 that would cause the unstable unread counts from [MSC2654](matrix-org/matrix-spec-proposals#2654) to be calculated even if the feature is disabled. ([\matrix-org#13694](matrix-org#13694)) Updates to the Docker image --------------------------- - Update docker image to use a stable version of poetry. ([\matrix-org#13688](matrix-org#13688)) Improved Documentation ---------------------- - Improve the description of the ["chain cover index"](https://matrix-org.github.io/synapse/latest/auth_chain_difference_algorithm.html) used internally by Synapse. ([\matrix-org#13602](matrix-org#13602)) - Document how ["monthly active users"](https://matrix-org.github.io/synapse/latest/usage/administration/monthly_active_users.html) is calculated and used. ([\matrix-org#13617](matrix-org#13617)) - Improve documentation around user registration. ([\matrix-org#13640](matrix-org#13640)) - Remove documentation of legacy `frontend_proxy` worker app. ([\matrix-org#13645](matrix-org#13645)) - Clarify documentation that HTTP replication traffic can be protected with a shared secret. ([\matrix-org#13656](matrix-org#13656)) - Remove unintentional colons from [config manual](https://matrix-org.github.io/synapse/latest/usage/configuration/config_documentation.html) headers. ([\matrix-org#13665](matrix-org#13665)) - Update docs to make enabling metrics more clear. ([\matrix-org#13678](matrix-org#13678)) - Clarify `(room_id, event_id)` global uniqueness and how we should scope our database schemas. ([\matrix-org#13701](matrix-org#13701)) Deprecations and Removals ------------------------- - Drop support for calling `/_matrix/client/v3/rooms/{roomId}/invite` without an `id_access_token`, which was not permitted by the spec. Contributed by @Vetchu. ([\matrix-org#13241](matrix-org#13241)) - Remove redundant `_get_joined_users_from_context` cache. Contributed by Nick @ Beeper (@Fizzadar). ([\matrix-org#13569](matrix-org#13569)) - Remove the ability to use direct TCP replication with workers. Direct TCP replication was deprecated in Synapse v1.18.0. Workers now require using Redis. ([\matrix-org#13647](matrix-org#13647)) - Remove support for unstable [private read receipts](matrix-org/matrix-spec-proposals#2285). ([\matrix-org#13653](matrix-org#13653), [\matrix-org#13692](matrix-org#13692)) Internal Changes ---------------- - Extend the release script to wait for GitHub Actions to finish and to be usable as a guide for the whole process. ([\matrix-org#13483](matrix-org#13483)) - Add experimental configuration option to allow disabling legacy Prometheus metric names. ([\matrix-org#13540](matrix-org#13540)) - Cache user IDs instead of profiles to reduce cache memory usage. Contributed by Nick @ Beeper (@Fizzadar). ([\matrix-org#13573](matrix-org#13573), [\matrix-org#13600](matrix-org#13600)) - Optimize how Synapse calculates domains to fetch from during backfill. ([\matrix-org#13575](matrix-org#13575)) - Comment about a better future where we can get the state diff between two events. ([\matrix-org#13586](matrix-org#13586)) - Instrument `_check_sigs_and_hash_and_fetch` to trace time spent in child concurrent calls for understandable traces in Jaeger. ([\matrix-org#13588](matrix-org#13588)) - Improve performance of `@cachedList`. ([\matrix-org#13591](matrix-org#13591)) - Minor speed up of fetching large numbers of push rules. ([\matrix-org#13592](matrix-org#13592)) - Optimise push action fetching queries. Contributed by Nick @ Beeper (@Fizzadar). ([\matrix-org#13597](matrix-org#13597)) - Rename `event_map` to `unpersisted_events` when computing the auth differences. ([\matrix-org#13603](matrix-org#13603)) - Refactor `get_users_in_room(room_id)` mis-use with dedicated `get_current_hosts_in_room(room_id)` function. ([\matrix-org#13605](matrix-org#13605)) - Use dedicated `get_local_users_in_room(room_id)` function to find local users when calculating `join_authorised_via_users_server` of a `/make_join` request. ([\matrix-org#13606](matrix-org#13606)) - Refactor `get_users_in_room(room_id)` mis-use to lookup single local user with dedicated `check_local_user_in_room(...)` function. ([\matrix-org#13608](matrix-org#13608)) - Drop unused column `application_services_state.last_txn`. ([\matrix-org#13627](matrix-org#13627)) - Improve readability of Complement CI logs by printing failure results last. ([\matrix-org#13639](matrix-org#13639)) - Generalise the `@cancellable` annotation so it can be used on functions other than just servlet methods. ([\matrix-org#13662](matrix-org#13662)) - Introduce a `CommonUsageMetrics` class to share some usage metrics between the Prometheus exporter and the phone home stats. ([\matrix-org#13671](matrix-org#13671)) - Add some logging to help track down matrix-org#13444. ([\matrix-org#13679](matrix-org#13679)) - Update poetry lock file for v1.2.0. ([\matrix-org#13689](matrix-org#13689)) - Add cache to `is_partial_state_room`. ([\matrix-org#13693](matrix-org#13693)) - Update the Grafana dashboard that is included with Synapse in the `contrib` directory. ([\matrix-org#13697](matrix-org#13697)) - Only run trial CI on all python versions on non-PRs. ([\matrix-org#13698](matrix-org#13698)) - Fix typechecking with latest types-jsonschema. ([\matrix-org#13712](matrix-org#13712)) - Reduce number of CI checks we run for PRs. ([\matrix-org#13713](matrix-org#13713)) # -----BEGIN PGP SIGNATURE----- # # iQFEBAABCgAuFiEEBTGR3/RnAzBGUif3pULk7RsPrAkFAmMgR2QQHGVyaWtAbWF0 # cml4Lm9yZwAKCRClQuTtGw+sCfG7B/94PwW1ChsaI8hkz/3e+93PEl/mNJ6YFaEB # 5pP4Dh/0dipP/iKbpgNuj5xz/JFnIi8D49A8sKNnku3jk0/8AZHgqDiBgOkrN76z # Y3awo5Q9ag4xww/105V3bhdnX1NrX8Avf6F2jchDv6/9q8wQHGBPg6DMgfZ/m/BL # SB4dypbbNpgLykuwtWxx6YMUYH+trsXJOn/MoAqld3QcZsqkDR25wXCt9+Dr+6AT # dPd/czi8kV8ruU59tf2K5HB7XKzBW9S3Qb3dJJmGOTTJ7ccUkN/XuTwqnII950Mo # bSlMXjY2hqk8rKUNhGZpi9bqUkwNhMgOkZl9A0Y1XtsXx6yjy0T/ # =zSGi # -----END PGP SIGNATURE----- # gpg: Signature made Tue Sep 13 10:03:32 2022 BST # gpg: using RSA key 053191DFF4670330465227F7A542E4ED1B0FAC09 # gpg: issuer "[email protected]" # gpg: Can't check signature: No public key # Conflicts: # synapse/config/experimental.py # synapse/push/bulk_push_rule_evaluator.py # synapse/storage/databases/main/event_push_actions.py # synapse/util/caches/descriptors.py

Remove cached wrap on _get_joined_users_from_context method

d68bda3

The method doesn't actually do any data fetching and the method that does, `_get_joined_profile_from_event_id`, has it's own cache.

Fizzadar requested a review from a team as a code owner August 19, 2022 12:45

Fizzadar added 5 commits August 19, 2022 13:45

Changelog file

4346425

Remove context argument from _get_joined_users_from_context method

caab2e1

If the cache hits are indeed next to zero for this method, the context optimisation provides little benefit and thus can be removed.

Remove unused event argument from _get_joined_users_from_context

c4b32ba

Linting

b63692d

Remove unused room_id argument

f992ddf

richvdh self-requested a review August 24, 2022 08:56

richvdh reviewed Aug 24, 2022

View reviewed changes

richvdh suggested changes Aug 24, 2022

View reviewed changes

Fizzadar added 4 commits August 25, 2022 09:43

Remove state_group and entry arguments from `get_joined_users_fro…

5f99fe8

…m_state` methods

Inline _get_joined_users_from_context method

63967da

Update changelog

d5a5f7f

Merge branch 'develop' into remove-cached-get-joined-users-from-context

8379222

# Conflicts: # synapse/state/__init__.py # synapse/storage/databases/main/roommember.py

Fizzadar requested a review from richvdh August 25, 2022 08:54

richvdh suggested changes Aug 25, 2022

View reviewed changes

Fizzadar added 2 commits August 25, 2022 15:28

Fix mergem missing method rename

6404481

Fix formatting

4e62c46

richvdh self-requested a review August 25, 2022 15:28

richvdh reviewed Aug 25, 2022

View reviewed changes

Tidy up bad docstring

d630c0c

richvdh self-requested a review August 26, 2022 15:55

richvdh approved these changes Aug 31, 2022

View reviewed changes

richvdh merged commit 42b11d5 into matrix-org:develop Aug 31, 2022

Fizzadar deleted the remove-cached-get-joined-users-from-context branch August 31, 2022 13:21

richvdh mentioned this pull request Sep 22, 2022

Regression in federation sender instance calls to get joined profile #13444

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove cached wrap on `_get_joined_users_from_context` method #13569

Remove cached wrap on `_get_joined_users_from_context` method #13569

Fizzadar commented Aug 19, 2022 •

edited

Loading

richvdh Aug 24, 2022

richvdh Aug 24, 2022

richvdh left a comment

richvdh Aug 24, 2022

Fizzadar Aug 25, 2022

richvdh Aug 24, 2022

Fizzadar Aug 25, 2022

richvdh Aug 24, 2022

richvdh Aug 24, 2022

Fizzadar Aug 25, 2022

Fizzadar commented Aug 25, 2022

richvdh Aug 25, 2022

Fizzadar Aug 25, 2022

richvdh left a comment

richvdh Aug 25, 2022

richvdh Aug 25, 2022

Fizzadar Aug 26, 2022

richvdh left a comment

		# We don't use `state_group`, it's there so that we can cache based
		# on it. However, it's important that it's never None, since two current_states


		@cached(num_args=2, iterable=True, max_entries=100000)
		async def _get_joined_users_from_context(

		@@ -0,0 +1 @@
		Remove cache on `_get_joined_users_from_context` calls that are already cached elsewhere. Contributed by Nick @ Beeper (@fizzadar).

		For a given set of state IDs, get a map of user ID to profile information
		for users in the room.

Remove cached wrap on _get_joined_users_from_context method #13569

Remove cached wrap on _get_joined_users_from_context method #13569

Conversation

Fizzadar commented Aug 19, 2022 • edited Loading

Pull Request Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richvdh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fizzadar commented Aug 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richvdh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richvdh left a comment

Choose a reason for hiding this comment

Remove cached wrap on `_get_joined_users_from_context` method #13569

Remove cached wrap on `_get_joined_users_from_context` method #13569

Fizzadar commented Aug 19, 2022 •

edited

Loading