Skip to content
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

look up cross-signing keys from the DB in bulk #6486

Merged
merged 16 commits into from Dec 12, 2019
Merged

Conversation

@uhoreg
Copy link
Member

uhoreg commented Dec 6, 2019

to improve performance of looking up 500 users' keys.

Also improve some docstrings.

uhoreg added 6 commits Dec 6, 2019
to improve performance of looking up 500 users' keys.

Also improve some docstrings.
@uhoreg uhoreg requested a review from matrix-org/synapse-core Dec 6, 2019
Copy link
Member

erikjohnston left a comment

Thanks! It'd be good to also add a cache here, as e.g. if a room enables encryption then we'll get N users asking for the keys of the other N users, so adding a cache should help a lot. (c.f. @cachedList)

synapse/storage/data_stores/main/end_to_end_keys.py Outdated Show resolved Hide resolved
synapse/storage/data_stores/main/end_to_end_keys.py Outdated Show resolved Hide resolved
synapse/storage/data_stores/main/end_to_end_keys.py Outdated Show resolved Hide resolved
synapse/storage/data_stores/main/end_to_end_keys.py Outdated Show resolved Hide resolved
synapse/storage/data_stores/main/end_to_end_keys.py Outdated Show resolved Hide resolved
synapse/handlers/e2e_keys.py Outdated Show resolved Hide resolved
@uhoreg uhoreg requested a review from erikjohnston Dec 6, 2019
@uhoreg

This comment has been minimized.

Copy link
Member Author

uhoreg commented Dec 7, 2019

It'd be good to also add a cache here, as e.g. if a room enables encryption then we'll get N users asking for the keys of the other N users, so adding a cache should help a lot. (c.f. @cachedList)

Ah, I forgot to do this.

So the difficulty here is that the from_user_id will be different for each user, which would make the result different, so we can't just cache the whole thing. I think we'd have to split the first query out into a separate function, cache that, and then add in the signatures afterwards.

I think that it'll end up being a big change, so I'll take it out of review until it's done.

@uhoreg

This comment has been minimized.

Copy link
Member Author

uhoreg commented Dec 10, 2019

I've split out the first query into a separate function, and set it up to take a list of key types. However, I'm running into some issues adding the caching.

  1. I did the split in the function at https://github.com/matrix-org/synapse/pull/6486/files#diff-9f5f1ff9d5f3d4e7194db99c80985c81R340 which takes a txn parameter. Is it possible to make @cachedList ignore the txn parameter, or do I need to apply @cachedList to the non-txn version (which means the two queries will be in separate transactions)?
  2. If I allow it to take a list of key types to fetch, then that seems like it could cause the cache to explode, as we could have a cache entry for each user, for each possible subset of key types. This would also make cache invalidation hard. In practice, we currently only search for both master and self-signing keys with that function, but it seems a bit dangerous to rely on that always being true. Are there any suggestions for how to deal with this?
@erikjohnston

This comment has been minimized.

Copy link
Member

erikjohnston commented Dec 10, 2019

  1. I did the split in the function at https://github.com/matrix-org/synapse/pull/6486/files#diff-9f5f1ff9d5f3d4e7194db99c80985c81R340 which takes a txn parameter. Is it possible to make @cachedList ignore the txn parameter, or do I need to apply @cachedList to the non-txn version (which means the two queries will be in separate transactions)?

Yeah, I'm afraid they will need to be in separate transactions (and you can then cache both transactions). I don't think that'll cause too much overhead as its just one extra query per request, rather than an extra query per queried user?

  1. If I allow it to take a list of key types to fetch, then that seems like it could cause the cache to explode, as we could have a cache entry for each user, for each possible subset of key types. This would also make cache invalidation hard. In practice, we currently only search for both master and self-signing keys with that function, but it seems a bit dangerous to rely on that always being true. Are there any suggestions for how to deal with this?

I think including the key type is fine, the default cache sizes aren't that big so it won't use that much extra memory by default. You can then use invalidate_many to invalidate all keys for a user (or the standard invalidate to invalidatejust a particular user/key). c.f._get_linearized_receipts_for_room` for an example

The other option is to have the query return all keys for a user, filter the keys after the cache, and then invalidate all keys for a user if one of them changes. It's not perfect, but probably good enough.

@uhoreg uhoreg self-assigned this Dec 10, 2019
@uhoreg

This comment has been minimized.

Copy link
Member Author

uhoreg commented Dec 10, 2019

Tests were working fine until I merged in develop, and then get_e2e_cross_signing_keys_bulk is now infinite-looping. (e.g. f1ea148 should work fine). From my debugging (i.e. print statements), it seems to be hanging somewhere between the return statement in _get_bare_e2e_cross_signing_keys_bulk_txn and the value arriving into _get_bare_e2e_cross_signing_keys_bulk. i.e., it seems to be hanging somewhere in runInteraction. Has something changed there recently?

Anyways, aside from that, should be ready for review now.

@uhoreg uhoreg requested a review from erikjohnston Dec 10, 2019
@erikjohnston

This comment has been minimized.

Copy link
Member

erikjohnston commented Dec 11, 2019

Looks like it might be exploding due to a leaked log context, somewhere

@uhoreg

This comment has been minimized.

Copy link
Member Author

uhoreg commented Dec 11, 2019

Tests pass now. Should be good now. There is a bit of ugliness, though, in that the @cachedList seems to add None values to the dict, which need to be skipped over by various things. So there's a question of whether we add code to skip over None values, or whether we make another iteration of the dict in get_e2e_cross_signing_keys_bulk to drop the None values. I've implemented the first option for now, but switching should be easy. Aside from that minor thing, I think this should be ready.

@uhoreg uhoreg requested a review from erikjohnston Dec 11, 2019
list_name="user_ids",
num_args=1,
)
def _get_bare_e2e_cross_signing_keys_bulk(self, user_ids: list) -> dict:

This comment has been minimized.

Copy link
@erikjohnston

erikjohnston Dec 12, 2019

Member
Suggested change
def _get_bare_e2e_cross_signing_keys_bulk(self, user_ids: list) -> dict:
def _get_bare_e2e_cross_signing_keys_bulk(self, user_ids: List[str]) -> Dict[str, Dict[str, dict]]:

(You might need to import from typing)

the signatures for the calling user need to be fetched.
Args:
txn (twisted.enterprise.adbapi.Connection): db connection

This comment has been minimized.

Copy link
@erikjohnston

erikjohnston Dec 12, 2019

Member
Suggested change
txn (twisted.enterprise.adbapi.Connection): db connection
@@ -271,7 +271,7 @@ def __init__(
else:
self.function_to_call = orig

arg_spec = inspect.getargspec(orig)
arg_spec = inspect.getfullargspec(orig)

This comment has been minimized.

Copy link
@erikjohnston

This comment has been minimized.

Copy link
@uhoreg

uhoreg Dec 12, 2019

Author Member

Right, I was going to add a comment explaining it. It's because _get_bare_e2e_cross_signing_keys_bulk has type annotations, so getargspec doesn't work on it, and it suggests to use getfullargspec instead.

This comment has been minimized.

Copy link
@erikjohnston

erikjohnston Dec 12, 2019

Member

aaaaaah, cool

@neilisfragile neilisfragile moved this from In progress to Review in Homeserver Task Board Dec 12, 2019
@uhoreg uhoreg merged commit cb2db17 into develop Dec 12, 2019
22 checks passed
22 checks passed
buildkite/synapse Build #6131 passed (20 minutes, 40 seconds)
Details
buildkite/synapse/check-sample-config Passed (1 minute, 47 seconds)
Details
buildkite/synapse/check-style Passed (1 minute, 52 seconds)
Details
buildkite/synapse/isort Passed (18 seconds)
Details
buildkite/synapse/mypy Passed (24 seconds)
Details
buildkite/synapse/newspaper-newsfile Passed (16 seconds)
Details
buildkite/synapse/packaging Passed (19 seconds)
Details
buildkite/synapse/pipeline Passed (4 seconds)
Details
buildkite/synapse/python-3-dot-5-slash-postgres-9-dot-5 Passed (18 minutes, 24 seconds)
Details
buildkite/synapse/python-3-dot-5-slash-sqlite Passed (6 minutes, 48 seconds)
Details
buildkite/synapse/python-3-dot-5-slash-sqlite-slash-old-deps Passed (7 minutes, 37 seconds)
Details
buildkite/synapse/python-3-dot-6-slash-sqlite Passed (7 minutes, 30 seconds)
Details
buildkite/synapse/python-3-dot-7-slash-postgres-11 Passed (18 minutes, 5 seconds)
Details
buildkite/synapse/python-3-dot-7-slash-postgres-9-dot-5 Passed (18 minutes, 37 seconds)
Details
buildkite/synapse/python-3-dot-7-slash-sqlite Passed (6 minutes, 37 seconds)
Details
buildkite/synapse/synapse-port-db-slash-python-3-dot-5-slash-postgres-9-dot-5 Passed (1 minute, 40 seconds)
Details
buildkite/synapse/synapse-port-db-slash-python-3-dot-7-slash-postgres-11 Passed (1 minute, 24 seconds)
Details
buildkite/synapse/sytest-python-3-dot-5-slash-postgres-9-dot-6-slash-monolith Passed (13 minutes, 33 seconds)
Details
buildkite/synapse/sytest-python-3-dot-5-slash-postgres-9-dot-6-slash-workers Passed (16 minutes, 10 seconds)
Details
buildkite/synapse/sytest-python-3-dot-5-slash-sqlite-slash-monolith Passed (11 minutes, 7 seconds)
Details
buildkite/synapse/sytest-python-3-dot-7-slash-postgres-11-slash-monolith Passed (12 minutes, 22 seconds)
Details
buildkite/synapse/sytest-python-3-dot-7-slash-postgres-11-slash-workers Passed (13 minutes, 42 seconds)
Details
Homeserver Task Board automation moved this from Review to Done Dec 12, 2019
@neilisfragile neilisfragile moved this from Done to temp done in Homeserver Task Board Dec 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.