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

Refactor state group lookup to reduce DB hits #4011

Merged
merged 38 commits into from Oct 25, 2018

Conversation

2 participants
@erikjohnston
Member

erikjohnston commented Oct 5, 2018

Currently when fetching state groups from the data store we make two
hits two the database: once for members and once for non-members (unless
request is filtered to one or the other). This adds needless load to the
datbase, so this PR refactors the lookup to make only a single database
hit.

erikjohnston added some commits Oct 5, 2018

Refactor state group lookup to reduce DB hits
Currently when fetching state groups from the data store we make two
hits two the database: once for members and once for non-members (unless
request is filtered to one or the other). This adds needless load to the
datbase, so this PR refactors the lookup to make only a single database
hit.

@erikjohnston erikjohnston requested a review from matrix-org/synapse-core Oct 8, 2018

@richvdh

I think I'm going to stop now. Suffice it to say that this stuff is fiddly.

I do wonder if we're massively over-complicating this by trying to find a general solution. It might be informative to audit how types and filtered_types are actually used, and consider if we ought to be caching at a different level, or something.

@@ -749,6 +749,9 @@ def _get_state_for_groups(self, groups, types=None, filtered_types=None):
Deferred[dict[int, dict[tuple[str, str], str]]]:
dict of state_group_id -> (dict of (type, state_key) -> event id)
"""
# First, lets split up the types and filtered types into non-member vs

This comment has been minimized.

@richvdh

richvdh Oct 9, 2018

Member

let's

This comment has been minimized.

@richvdh

richvdh Oct 9, 2018

Member

s/filtered types/filtered_types/

This comment has been minimized.

@richvdh

richvdh Oct 9, 2018

Member

this if statement currently does not split up filtered_types into non-member vs member. (It maybe should, to save problems further down)

groups, self._state_group_cache, non_member_types, filtered_types,
)
# XXX: we could skip this entirely if member_types is []
member_state = yield self._get_state_for_groups_using_cache(
non_member_state, missing_groups_nm, = r_nm

This comment has been minimized.

@richvdh

richvdh Oct 9, 2018

Member

can you not inline this, for less ugliness and fewer random locals?

non_member_state, missing_groups_nm = (
    yield self._get_state_for_groups_using_cache(
        groups, self._state_group_cache, non_member_types, filtered_types,
    )
)

This comment has been minimized.

@richvdh

richvdh Oct 9, 2018

Member

also, I think incomplete_groups might be a more appropriate name than missing_groups

@@ -802,13 +870,14 @@ def _get_state_for_groups_using_cache(
If None, `types` filtering is applied to all events.
Returns:
Deferred[dict[int, dict[tuple[str, str], str]]]:
dict of state_group_id -> (dict of (type, state_key) -> event id)
tuple[dict[int, dict[tuple[str, str], str]], set[dict]]: Tuple of

This comment has been minimized.

@richvdh

richvdh Oct 9, 2018

Member

s/set[dict]/set[int]/ afaict

dict of state_group_id -> (dict of (type, state_key) -> event id)
tuple[dict[int, dict[tuple[str, str], str]], set[dict]]: Tuple of
dict of state_group_id -> (dict of (type, state_key) -> event id)
of entries in the cache, and the state groups missing from the cache

This comment has been minimized.

@richvdh

richvdh Oct 9, 2018

Member

s/state groups/state_group ids I think

This comment has been minimized.

@richvdh

richvdh Oct 9, 2018

Member

worth noting that said state groups might not be entirely missing - we may just have incomplete data for them.

missing_groups = missing_groups_m | missing_groups_nm
if missing_groups:

This comment has been minimized.

@richvdh

richvdh Oct 9, 2018

Member

opinions vary on the desirability of this, so up to you, but I prefer an early bail-out than 100 lines of indent which I have to scroll through to find out if there's an else clause:

if not missing_groups:
    defer.returnValue(state)

(or better yet, factor out a separate function)

else:
types_to_fetch = types
non_member_types_fetched = [

This comment has been minimized.

@richvdh

richvdh Oct 9, 2018

Member

isn't this the same as non_member_types above? (except that types might be None, in which case this will explode)

This comment has been minimized.

@richvdh

richvdh Oct 9, 2018

Member

[I'm surprised this isn't making the UTs fail tbh]

This comment has been minimized.

@erikjohnston

erikjohnston Oct 10, 2018

Member

Hilariously it appears we never hit the non-cached code path, presumably because we insert into the cache on write...

This comment has been minimized.

@richvdh
non_member_types_fetched = [
t for t in types if t[0] != EventTypes.Member
]
member_types_fetched = [

This comment has been minimized.

@richvdh

richvdh Oct 9, 2018

Member

likewise, the same as member_types?

Show resolved Hide resolved synapse/storage/state.py
else:
state_dict.update(group_state_dict)
# update the cache with all the things we fetched from the

This comment has been minimized.

@richvdh

richvdh Oct 9, 2018

Member

this is a bit redundant now

if k in types_fetched or (typ, None) in types_fetched:
state_dict[k] = v
else:
state_dict.update(group_state_dict)

This comment has been minimized.

@richvdh

richvdh Oct 9, 2018

Member

does this not need splitting by membership/non-membership?

This comment has been minimized.

@richvdh

richvdh Oct 9, 2018

Member

also: I'm not sure there's any point in updating the existing dict, rather than just overwriting it

@erikjohnston

This comment has been minimized.

Member

erikjohnston commented Oct 10, 2018

I do wonder if we're massively over-complicating this by trying to find a general solution. It might be informative to audit how types and filtered_types are actually used, and consider if we ought to be caching at a different level, or something.

Absolutely we are over-complicating this, filtered_types is only ever None or [EventTypes.Member] afaict, but I'd quite like to avoid rewriting all the layers right now.

@erikjohnston erikjohnston requested a review from matrix-org/synapse-core Oct 12, 2018

@erikjohnston

This comment has been minimized.

Member

erikjohnston commented Oct 12, 2018

I've changed a bunch of the code to bundle a lot of the logic in handling types/filtered_types into a StateFilter type. I haven't changed the public interfaces to use it, but that might be the logical conclusion in making this a bit nicer.

One problem here is that currently the unit tests do not test these code paths very well, since we prefill our state caches on insertion. Commenting out the prefill will test the code correctly. The question is, how do we want to ensure the code is tested while keeping the behaviour the same in production? We can explicitly clear the caches after insertion in our storage state caches, or have a parameter that turns off prefilling?

@erikjohnston erikjohnston removed their assignment Oct 15, 2018

@erikjohnston has changed things

@richvdh

oh gosh. I'd hoped that if we refactored all this to use a separate Filter object, we could avoid the whole headfuck of the types/filtered_types thing.

There are multiple ways to do this (one option would be an interface which could be implemented by various implementations like a AllMembersStateFilter or a LazyLoadMembersStateFilter), but
I think a more practical impl would be to make the attributes member_types and non_member_types and provide some helper functions to construct the objects.

And yeah, I know that means rewriting a lot more stuff :/

"""Split the filter into member vs non-member types.
Returns:
tuple[Iterable[str, str|None]|None]: Returns a tuple of member,

This comment has been minimized.

@richvdh

richvdh Oct 23, 2018

Member

this type suggests it is a 1-tuple. Also, it returns lists and I think you should say so:

tuple[List[str, str|None]|None, List[str, str|None]|None]

... yeah, I know.

This comment has been minimized.

@richvdh

richvdh Oct 23, 2018

Member

having said that: why not just split it into get_member_types (which could return just a list[str]|None for the state_keys) and get_non_member_types?

This comment has been minimized.

@richvdh

richvdh Oct 23, 2018

Member

having said having said that: I think we should do this during construction, as per the above.

non-member types in the same format as `types`.
"""
if self.types is not None:
non_member_types = [t for t in self.types if t[0] != EventTypes.Member]

This comment has been minimized.

@richvdh

richvdh Oct 23, 2018

Member

this needs to check filtered_types, I think

types is not None and
not wildcard_types and
len(results[group]) == len(types)
max_entries_returned and

This comment has been minimized.

@richvdh

richvdh Oct 23, 2018

Member

this is missing an is not None

The default, `StateFilter()`, is equivalent to no filter.
Attributes:
types (Iterable[str, str|None]|None): list of 2-tuples of the form

This comment has been minimized.

@richvdh

richvdh Oct 23, 2018

Member

afaict we iterate over this stuff multiple times, so a plain iterable won't do. s/Iterable/list/, possibly.

Show resolved Hide resolved synapse/storage/state.py
# We want to return everything.
return StateFilter()
else:
# We want to return all members, but only the specified types

This comment has been minimized.

@richvdh

richvdh Oct 23, 2018

Member

s/specified types/specified non-member types/

if self.types is None:
return where_clause, where_args
types = set(self.types)

This comment has been minimized.

@richvdh

richvdh Oct 23, 2018

Member

this seems a bit redundant. We know self.types is going to be a list.

@@ -391,62 +563,17 @@ def _get_state_groups_from_groups_txn(
%s

This comment has been minimized.

@richvdh

richvdh Oct 23, 2018

Member

can we just do """..."""" + where_clause rather than interpolating the where clause later?

erikjohnston added some commits Oct 24, 2018

Refactor StateFilter to be cleaner
This changes the internal representation of `StateFilter` to remove the
concept of `filtered_types` in favour of a `types` dict and a flag to
indicate whether or not to fetch types not in the `types` dict.

Ideally in the future we should be able to rewrite our caching so that
we cache the filter used when fetching state from the DB. Then when we
go fetch more state we can compare the given filter with the filter of
the cached result to see if the given is a subset of the cached filter.
This would avoid the convolutions of maintaining separate caches.
# (if we are) to fix https://github.com/vector-im/riot-web/issues/7209
# We only need apply this on full state syncs given we disabled
# LL for incr syncs in #3840.
members_to_fetch.add(sync_config.user.to_string())

This comment has been minimized.

@erikjohnston

erikjohnston Oct 24, 2018

Member

(Note this has been moved up from just below so we can work out members_to_fetch in a single place)

@erikjohnston

This comment has been minimized.

Member

erikjohnston commented Oct 24, 2018

Right, first off, I'm so sorry I regret ever setting eyes on filtered_types 😢

On the other hand, filtered_types no longer appear anywhere in the code base 🎉

In the end I didn't change StateFilter to internally store the split of member vs non member stuff, as it turned out to be quite fiddly. Instead it has a types dict mapping from event type to set of state keys (or None) that indicates specifically which types/state_keys to include/exclude, and a flag to indicate whether to fetch types that don't appear in the types dict.

Ideally we could then easily change the cache to store the StateFilter used when fetching state, which would mean on look up we could simply compare the given state filter with the one stored in the cache. (I believe checking if one state filter is a subset of another is easy enough to implement). This would allows us to rip out all the special casing for membership events in the state store.

Everything would then be fine and lovely and we should never touch the state store ever again.

@erikjohnston erikjohnston requested a review from richvdh Oct 24, 2018

@richvdh

this is generally looking much saner. Sorry about the number of comments but I think they're all fairly easy to resolve.

Show resolved Hide resolved synapse/storage/state.py
Show resolved Hide resolved synapse/storage/state.py Outdated
Show resolved Hide resolved synapse/storage/state.py
or any(
s is None
for t, state_keys in iteritems(self.types)
for s in state_keys

This comment has been minimized.

@richvdh

richvdh Oct 25, 2018

Member

this looks like it is looking for None in the set of state_keys (which is meaningless), rather than the set itself being None

self.include_others
or any(
s is None
for t, state_keys in iteritems(self.types)

This comment has been minimized.

@richvdh

richvdh Oct 25, 2018

Member

use itervalues if you're not going to use the key?

where_clause, where_args = state_filter.make_sql_filter_clause()
if where_clause:
where_clause = " AND (%s)" % (where_clause,)

This comment has been minimized.

@richvdh

richvdh Oct 25, 2018

Member

I think I'd find this clearer:

Suggested change Beta
where_clause = " AND (%s)" % (where_clause,)
sql += " AND (%s)" % (where_clause,)
Show resolved Hide resolved synapse/storage/state.py
@@ -322,20 +583,14 @@ def get_state_groups(self, room_id, event_ids):
})
@defer.inlineCallbacks
def _get_state_groups_from_groups(self, groups, types, members=None):
def _get_state_groups_from_groups(self, groups, state_filter=StateFilter.all()):

This comment has been minimized.

@richvdh

richvdh Oct 25, 2018

Member

given that this is a private method, and a value is always passed for state_filter, I would be inclined to drop the default

type_to_key.setdefault(typ, set()).add(state_key)
if state_filter.has_wildcards():
# we mark the type as missing from the cache because

This comment has been minimized.

@richvdh

richvdh Oct 25, 2018

Member

this conment is a bit unclear now

Show resolved Hide resolved synapse/storage/state.py
Fix docstring typo
Co-Authored-By: erikjohnston <erikj@jki.re>

richvdh and others added some commits Oct 25, 2018

Fix up docstring
Co-Authored-By: erikjohnston <erikj@jki.re>
Fix up docstring
Co-Authored-By: erikjohnston <erikj@jki.re>
Fix up docstring
Co-Authored-By: erikjohnston <erikj@jki.re>
Fix up docstring
Co-Authored-By: erikjohnston <erikj@jki.re>
Fix up docstring
Co-Authored-By: erikjohnston <erikj@jki.re>
Fix using sum rather than len
Co-Authored-By: erikjohnston <erikj@jki.re>

@erikjohnston erikjohnston requested a review from richvdh Oct 25, 2018

@erikjohnston

This comment has been minimized.

Member

erikjohnston commented Oct 25, 2018

I think that should have addressed all the comments :)

@richvdh

lgtm otherwise. please fix up and merge

Show resolved Hide resolved synapse/storage/state.py
@@ -328,6 +350,15 @@ def get_member_split(self):
return member_filter, non_member_filter
def __attrs_post_init__(self):

This comment has been minimized.

@richvdh

richvdh Oct 25, 2018

Member

could this go at the top, where __init__ would normally go?

@erikjohnston erikjohnston merged commit cb53ce9 into develop Oct 25, 2018

5 checks passed

ci/circleci: sytestpy2merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy2postgresmerged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3postgresmerged Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@neilisfragile neilisfragile added this to To Do in Backend Core Team via automation Oct 25, 2018

@neilisfragile neilisfragile moved this from To Do to Done - Operations in Backend Core Team Oct 25, 2018

@erikjohnston erikjohnston referenced this pull request Oct 29, 2018

Closed

TypeError on /search #4112

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment