Speed up lazy loading by ara4n · Pull Request #3827 · matrix-org/synapse

ara4n · 2018-09-07T23:14:05Z

Rather than pulling room member events out of state storage to calculate room summaries, instead generate it from the room_memberships table.

…rate queries.

ara4n · 2018-09-07T23:14:17Z

(this speeds up LL by about 2-3x)

synapse/storage/events.py

erikjohnston

I've only had a chance to look at the lazy loading bits of this. Can you please split out the fixes to get_filtered_current_state_ids to a separate PR please?

Things look broadly fine, other than its quite confusing trying to keep track of what r is any given situation. Hopefully the suggested changes will help clear things up a bit.

The only major thing is whether we are currently pulling the members out of the database. It feels odd to pull out 6 arbitrary users regardless of membership, while it seems the sync code assumes that e.g. if there are joined users in the room MemberSummary.members will be nonempty

erikjohnston · 2018-09-10T13:01:54Z

synapse/storage/roommember.py

+                " AND m.user_id = c.state_key"
+                " WHERE c.type = 'm.room.member' AND c.room_id = ?"
+                " GROUP BY m.membership"
+            )


FYI you can write:

sql = """ SELECT count(*), m.membership FROM room_memberships as m INNER JOIN current_state_events as c ON m.event_id = c.event_id AND m.room_id = c.room_id AND m.user_id = c.state_key WHERE c.type = 'm.room.member' AND c.room_id = ? GROUP BY m.membership """

rather than faffing around with quotes. It has the added benefit that it makes it much easier to c+p :)

agreed; this was just trying to be consistent with what was already there.

erikjohnston · 2018-09-10T13:03:41Z

synapse/storage/roommember.py

+            txn.execute(sql, (room_id,))
+            res = {}
+            for r in txn:
+                summary = res.setdefault(to_ascii(r[1]), MemberSummary([], r[0]))


It'd be nicer to write

for count, membership in txn: ...

so that its easier to verify that things match the intent

erikjohnston · 2018-09-10T13:04:33Z

synapse/storage/roommember.py

+                dict of membership states, pointing to a MemberSummary named tuple.
+        """
+
+        def f(txn):


Please name these something like _get_room_summary_txn or whatever, it makes stack traces a lot easier to understand

(this was following the pattern already present, fwiw)

erikjohnston · 2018-09-10T13:06:33Z

synapse/storage/roommember.py

+            txn.execute(sql, (room_id, 6))
+            for r in txn:
+                summary = res.get(to_ascii(r[1]))
+                members = summary.members


Won't this explode if summary is None?

I suppose it'll definitely not be None due the shape of the queries, but then we should probably not use get to make the intent a bit clearer.

synapse/storage/events.py

erikjohnston · 2018-09-10T13:39:35Z

synapse/handlers/sync.py

+            Membership.BAN
+        ):
+            for r in details.get(m, ([], 0))[0]:
+                member_ids[r[0]] = r[1]


I'm really not following what this is doing. Can we choose variable names and/or unpack tuples to help clarity please?

Unpacking this a bit seems to give:

for user_id, event_id in details.get(membership, empty_ms).members: member_ids[user_id] = event_id

I think?

erikjohnston · 2018-09-10T13:47:01Z

synapse/storage/roommember.py


+# "members" points to a truncated list of (user_id, event_id) tuples for users of
+# a given membership type, suitable for use in calculating heroes for a room.
+# "count" points to the total numberr of users of a given membership type.


Is there a reason that members is a list of tuples rather than a dict?

yes; i was worried about creating a new large cache in addition to the one we already maintain of room members, and so wanted it to be small (i'm assuming that tuples take up less room as they don't include key strings).

(i'm assuming that tuples take up less room as they don't include key strings)

Well this tuple is including the same number of strings, right? It might save a bit by not including the hash map I guess

If we're worried about the size of the cache then what we can do is have get_room_summary return an object with a custom __len__ implementation that returns the sum of the sizes of MemberSummary.members. This would ensure that the total number of tuples stored by the cache is less than the given cache size (as opposed to the number of room summaries being less than the given cache size)

synapse/handlers/sync.py

erikjohnston · 2018-09-10T13:56:26Z

synapse/handlers/sync.py

+                    # XXX: this may include false positives in the form of LL
+                    # members which have snuck into state
+                    batch.limited and
+                    any(t == EventTypes.Member for (t, k) in state.keys())


Please don't use .keys() or .items() etc, they are expensive in py2 due to them copying the data into a list. Either use six.iteritems etc to get iterators, or in this case simply drop .keys() as iterating over a dict iterates over the keys anyway.

erikjohnston · 2018-09-10T13:57:47Z

synapse/handlers/sync.py

+                    any(t == EventTypes.Member for (t, k) in state.keys())
+                ) or
                since_token is None
            )


Also, can this have a comment to explain why we're not always including the summary and when we do include a summary? The logic is non-trivial and its hard to know whether its right or not without a guiding comment.

ara4n · 2018-09-10T16:22:51Z

Can you please split out the fixes to get_filtered_current_state_ids to a separate PR please?

done, it's back in #3792

The only major thing is whether we are currently pulling the members out of the database. It feels odd to pull out 6 arbitrary users regardless of membership, while it seems the sync code assumes that e.g. if there are joined users in the room MemberSummary.members will be nonempty

Right. i guess it needs an order by membership in there to make sure we prefer joined users over invited ones and only use left users in a disaster.

ara4n · 2018-09-10T17:20:47Z

@erikjohnston ptal

ara4n · 2018-09-11T11:16:41Z

i got an IRL lgtm from @erikjohnston on this. i think.

See element-hq/element-web#7211 for details

ara4n · 2018-09-11T11:23:07Z

n.b. this is now twinned with matrix-org/sytest#489

erikjohnston

Modulo perhaps wanting to further pin down the ordering of heroes

ara4n · 2018-09-11T23:49:31Z

the failed test here seems to be unrelated; merging anyway.

Features -------- - Python 3.5 and 3.6 support is now in beta. ([\#3576](#3576)) - Implement `event_format` filter param in `/sync` ([\#3790](#3790)) - Add synapse_admin_mau:registered_reserved_users metric to expose number of real reaserved users ([\#3846](#3846)) Bugfixes -------- - Remove connection ID for replication prometheus metrics, as it creates a large number of new series. ([\#3788](#3788)) - guest users should not be part of mau total ([\#3800](#3800)) - Bump dependency on pyopenssl 16.x, to avoid incompatibility with recent Twisted. ([\#3804](#3804)) - Fix existing room tags not coming down sync when joining a room ([\#3810](#3810)) - Fix jwt import check ([\#3824](#3824)) - fix VOIP crashes under Python 3 (#3821) ([\#3835](#3835)) - Fix manhole so that it works with latest openssh clients ([\#3841](#3841)) - Fix outbound requests occasionally wedging, which can result in federation breaking between servers. ([\#3845](#3845)) - Show heroes if room name/canonical alias has been deleted ([\#3851](#3851)) - Fix handling of redacted events from federation ([\#3859](#3859)) - ([\#3874](#3874)) - Mitigate outbound federation randomly becoming wedged ([\#3875](#3875)) Internal Changes ---------------- - CircleCI tests now run on the potential merge of a PR. ([\#3704](#3704)) - http/ is now ported to Python 3. ([\#3771](#3771)) - Improve human readable error messages for threepid registration/account update ([\#3789](#3789)) - Make /sync slightly faster by avoiding needless copies ([\#3795](#3795)) - handlers/ is now ported to Python 3. ([\#3803](#3803)) - Limit the number of PDUs/EDUs per federation transaction ([\#3805](#3805)) - Only start postgres instance for postgres tests on Travis CI ([\#3806](#3806)) - tests/ is now ported to Python 3. ([\#3808](#3808)) - crypto/ is now ported to Python 3. ([\#3822](#3822)) - rest/ is now ported to Python 3. ([\#3823](#3823)) - add some logging for the keyring queue ([\#3826](#3826)) - speed up lazy loading by 2-3x ([\#3827](#3827)) - Improved Dockerfile to remove build requirements after building reducing the image size. ([\#3834](#3834)) - Disable lazy loading for incremental syncs for now ([\#3840](#3840)) - federation/ is now ported to Python 3. ([\#3847](#3847)) - Log when we retry outbound requests ([\#3853](#3853)) - Removed some excess logging messages. ([\#3855](#3855)) - Speed up purge history for rooms that have been previously purged ([\#3856](#3856)) - Refactor some HTTP timeout code. ([\#3857](#3857)) - Fix running merged builds on CircleCI ([\#3858](#3858)) - Fix typo in replication stream exception. ([\#3860](#3860)) - Add in flight real time metrics for Measure blocks ([\#3871](#3871)) - Disable buffering and automatic retrying in treq requests to prevent timeouts. ([\#3872](#3872)) - mention jemalloc in the README ([\#3877](#3877)) - Remove unmaintained "nuke-room-from-db.sh" script ([\#3888](#3888))

ara4n added 15 commits September 4, 2018 23:14

this will never have worked due to the query being split up into sepa…

7afd63c

…rate queries.

changelog

040f14b

wip for speeding up room summaries

119b8b6

fix state

e3d028a

typo

0bbcd07

fix typos

c630121

thinko

c5976d3

fix thinkos

671ff2c

thinko

6020c11

yet another typo

2d92d5a

yet another typo

6d836eb

fix uninitialised var

3bccef4

flake8 and make API more compact & docced

967fdfe

switch to named tuple

c39754a

grab sufficient hero material

1b04557

ara4n requested a review from erikjohnston September 7, 2018 23:14

ara4n added 2 commits September 8, 2018 00:18

changelog

2a4629b

isort

53a12bc

ara4n commented Sep 9, 2018

View reviewed changes

synapse/storage/events.py Show resolved Hide resolved

erikjohnston suggested changes Sep 10, 2018

View reviewed changes

erikjohnston assigned ara4n Sep 10, 2018

ara4n added 2 commits September 10, 2018 16:55

improve comment

1e73d6c

split out 3792 back whence it came

47a3d7c

incorporate review

0020712

ara4n assigned erikjohnston and unassigned ara4n Sep 10, 2018

sql that actually compiles

2527370

disable LL for incr syncs, and log incr sync stats (#3840)

c4aa9d6

See element-hq/element-web#7211 for details

erikjohnston approved these changes Sep 11, 2018

View reviewed changes

improve hero ordering

af8bdc7

ara4n merged commit b041115 into develop Sep 11, 2018

hawkowl deleted the matthew/speed-up-ll branch September 20, 2018 14:00

Uh oh!

Conversation

ara4n commented Sep 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ara4n commented Sep 7, 2018

Uh oh!

Uh oh!

erikjohnston left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ara4n commented Sep 10, 2018

Uh oh!

ara4n commented Sep 10, 2018

Uh oh!

ara4n commented Sep 11, 2018

Uh oh!

ara4n commented Sep 11, 2018

Uh oh!

erikjohnston left a comment

Choose a reason for hiding this comment

Uh oh!

ara4n commented Sep 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ara4n commented Sep 7, 2018 •

edited

Loading