caching on `have_seen_event` is somewhat broken

[richvdh]

As discovered in #13863 (comment) and discussed in #synapse-dev. Part of #13856

The cache on have_seen_event uses a TreeCache, which means the cache is structured as {room_id: {event_id: result}}. However, _have_seen_events_dict tries to use it with @cachedList, which stores entries as {(room_id, event_id): result}. As a result, caching is inefficient, and cache invalidation is unreliable.

[richvdh]

We suspect this has been the case ever since the cache was added to have_seen_event in #9953, but it would be interesting to test this to confirm.

Fixing this probably involves updating @cachedList to work correctly with TreeCaches?

[richvdh]

Relatedly: have_seen_events calls _have_seen_events_dict, so the thing gets cached twice in the same cache, once as a room_id -> event_id -> result tree, and the other as a (room_id, event_id) -> result.

[richvdh]

(Fortunately, have_seen_event is mostly an optimisation, so failure to invalidate is unlikely to have had too much visible effect other than inefficiency.)

[erikjohnston]

I think that the usage of @cachedList is wrong. It expects to be given the same number of arguments as the underlying cached function, just with one of the arguments being an iterable

synapse/synapse/storage/databases/main/events_worker.py

Lines 1485 to 1488 in cfe486b

	@cachedList(cached_method_name="have_seen_event", list_name="keys")
	async def _have_seen_events_dict(
	self, keys: Collection[Tuple[str, str]]
	) -> Dict[Tuple[str, str], bool]:

I think that should actually be:

 @cachedList(cached_method_name="have_seen_event", list_name="event_ids") 
 async def _have_seen_events_dict( 
     self, room_id: str, event_ids: Collection[str],
 ) -> Dict[str, bool]: 

[richvdh]

oh right, that's quite a lot easier.