Speed up cached function access by erikjohnston · Pull Request #2075 · matrix-org/synapse

erikjohnston · 2017-03-28T10:27:58Z

Calls to cached functions are starting to turn up in the flame graphs of matrix.org.

We improve it by doing two things:

Don't wrap the return value of the cache descriptors in a deferred if the cache has the full value. This does mean that the storage functions may now return either a deferred or an actual value, but pretty much everywhere uses yield func(..) which happily deals with it.
Remove the call from getcallargs from the inner function as that is quite expensive. We can pre-compute a bunch of stuff when we first set up the cache.

These two changes speed up cache access by 10x on my desktop.

richvdh · 2017-03-29T12:59:09Z

synapse/util/caches/descriptors.py

what's this for?

richvdh · 2017-03-29T13:00:01Z

synapse/util/async.py

I bet this will break something somewhere :/

to be clear: I'm not suggesting doing much about it, other than watching for breakage when it lands.

To be honest I'm concerned about this, but it makes cache hits much cheaper.

probably worth documenting it in a docstring.

richvdh · 2017-03-29T13:01:14Z

synapse/util/caches/descriptors.py

please can you document what this thing is and what type it has.

richvdh · 2017-03-29T13:05:17Z

synapse/util/logcontext.py

I am not in favour of further bodges on top of preserve_context_over_deferred, since afaict it is broken. Let me send you a counter-PR here.

richvdh · 2017-03-30T12:23:41Z

ok so this now has a conflict, and I think the preserve_context_over_deferred hackery isn't needed.

erikjohnston · 2017-03-30T12:40:04Z

Thanks for doing the preserve PR! Will rebase this in a bit

This is because getcallargs recomputes the getargspec, amongst other things, which we don't need to do as its already been done

erikjohnston · 2017-03-30T14:02:21Z

@richvdh PTAL

richvdh · 2017-03-30T14:30:24Z

synapse/util/caches/descriptors.py


+        # The arg spec of the wrapped function, see `inspect.getargspec` for
+        # the type.
+        self.arg_spec = arg_spec


I still don't think this is used?

Err, good point

... and yet it is still here...

richvdh · 2017-03-30T14:33:30Z

synapse/util/logcontext.py

    the deferred follow the synapse logcontext rules: try
    ``make_deferred_yieldable`` instead.
    """
+    if not isinstance(deferred, defer.Deferred):


now redundant afaict?

richvdh · 2017-03-30T14:34:10Z

synapse/util/caches/descriptors.py

                observer = result_d.observe()

-            return logcontext.make_deferred_yieldable(observer)
+            if isinstance(observer, defer.Deferred):


make_deferred_yieldable will work ok with a non-deferred, so I think this is redundant. otoh I guess it optimises the common path?

Because some of the speed up comes from not having to bounce through all the deferred stuff (which is much more complicated than just unwrapping to get the value) at the call sites.

Though we can also leave that to another PR

understood, leave it how it is

richvdh · 2017-03-30T14:36:11Z

synapse/util/async.py

probably worth documenting it in a docstring.

erikjohnston · 2017-03-30T16:07:27Z

The concern that changing a bunch of functions to sometimes return deferreds is a bit risky, though at worst it should only cause an exception to be raised when someone tries to add a callback to the value returned. (i.e., we're not going to get incorrect values coming out)

richvdh

lgtm apart from spurious self.arg_spec.

richvdh · 2017-03-30T20:09:26Z

synapse/util/caches/descriptors.py

                observer = result_d.observe()

-            return logcontext.make_deferred_yieldable(observer)
+            if isinstance(observer, defer.Deferred):


understood, leave it how it is

richvdh · 2017-03-30T20:10:29Z

synapse/util/caches/descriptors.py


+        # The arg spec of the wrapped function, see `inspect.getargspec` for
+        # the type.
+        self.arg_spec = arg_spec


... and yet it is still here...

@psaavedra

Changes in synapse v0.21.0 (2017-05-18) ======================================= No changes since v0.21.0-rc3 Changes in synapse v0.21.0-rc3 (2017-05-17) =========================================== Features: * Add per user rate-limiting overrides (PR matrix-org#2208) * Add config option to limit maximum number of events requested by ``/sync`` and ``/messages`` (PR matrix-org#2221) Thanks to @psaavedra! Changes: * Various small performance fixes (PR matrix-org#2201, matrix-org#2202, matrix-org#2224, matrix-org#2226, matrix-org#2227, matrix-org#2228, matrix-org#2229) * Update username availability checker API (PR matrix-org#2209, matrix-org#2213) * When purging, don't de-delta state groups we're about to delete (PR matrix-org#2214) * Documentation to check synapse version (PR matrix-org#2215) Thanks to @hamber-dick! * Add an index to event_search to speed up purge history API (PR matrix-org#2218) Bug fixes: * Fix API to allow clients to upload one-time-keys with new sigs (PR matrix-org#2206) Changes in synapse v0.21.0-rc2 (2017-05-08) =========================================== Changes: * Always mark remotes as up if we receive a signed request from them (PR matrix-org#2190) Bug fixes: * Fix bug where users got pushed for rooms they had muted (PR matrix-org#2200) Changes in synapse v0.21.0-rc1 (2017-05-08) =========================================== Features: * Add username availability checker API (PR matrix-org#2183) * Add read marker API (PR matrix-org#2120) Changes: * Enable guest access for the 3pl/3pid APIs (PR matrix-org#1986) * Add setting to support TURN for guests (PR matrix-org#2011) * Various performance improvements (PR matrix-org#2075, matrix-org#2076, matrix-org#2080, matrix-org#2083, matrix-org#2108, matrix-org#2158, matrix-org#2176, matrix-org#2185) * Make synctl a bit more user friendly (PR matrix-org#2078, matrix-org#2127) Thanks @APwhitehat! * Replace HTTP replication with TCP replication (PR matrix-org#2082, matrix-org#2097, matrix-org#2098, matrix-org#2099, matrix-org#2103, matrix-org#2014, matrix-org#2016, matrix-org#2115, matrix-org#2116, matrix-org#2117) * Support authenticated SMTP (PR matrix-org#2102) Thanks @DanielDent! * Add a counter metric for successfully-sent transactions (PR matrix-org#2121) * Propagate errors sensibly from proxied IS requests (PR matrix-org#2147) * Add more granular event send metrics (PR matrix-org#2178) Bug fixes: * Fix nuke-room script to work with current schema (PR matrix-org#1927) Thanks @zuckschwerdt! * Fix db port script to not assume postgres tables are in the public schema (PR matrix-org#2024) Thanks @jerrykan! * Fix getting latest device IP for user with no devices (PR matrix-org#2118) * Fix rejection of invites to unreachable servers (PR matrix-org#2145) * Fix code for reporting old verify keys in synapse (PR matrix-org#2156) * Fix invite state to always include all events (PR matrix-org#2163) * Fix bug where synapse would always fetch state for any missing event (PR matrix-org#2170) * Fix a leak with timed out HTTP connections (PR matrix-org#2180) * Fix bug where we didn't time out HTTP requests to ASes (PR matrix-org#2192) Docs: * Clarify doc for SQLite to PostgreSQL port (PR matrix-org#1961) Thanks @benhylau! * Fix typo in synctl help (PR matrix-org#2107) Thanks @HarHarLinks! * ``web_client_location`` documentation fix (PR matrix-org#2131) Thanks @matthewjwolff! * Update README.rst with FreeBSD changes (PR matrix-org#2132) Thanks @feld! * Clarify setting up metrics (PR matrix-org#2149) Thanks @encks!

erikjohnston requested a review from richvdh March 28, 2017 10:27

erikjohnston changed the title ~~Speed up the cache~~ Speed up cached function access Mar 28, 2017

erikjohnston force-pushed the erikj/cache_speed branch from 59ba000 to 25ae7fd Compare March 28, 2017 10:31

erikjohnston assigned richvdh Mar 28, 2017

richvdh reviewed Mar 29, 2017

View reviewed changes

richvdh assigned erikjohnston and unassigned richvdh Mar 30, 2017

erikjohnston added 4 commits March 30, 2017 14:14

Don't convert to deferreds when not necessary

86780a8

Manually calculate cache key as getcallargs is expensive

014fee9

This is because getcallargs recomputes the getargspec, amongst other things, which we don't need to do as its already been done

Fix up tests

eefd9fe

Doc new instance variables

6194a64

erikjohnston force-pushed the erikj/cache_speed branch from 25ae7fd to 6194a64 Compare March 30, 2017 13:19

erikjohnston assigned richvdh and unassigned erikjohnston Mar 30, 2017

richvdh suggested changes Mar 30, 2017

View reviewed changes

richvdh assigned erikjohnston and unassigned richvdh Mar 30, 2017

erikjohnston added 2 commits March 30, 2017 17:03

Revert log context change

b282fe7

Docs

5b5b171

richvdh approved these changes Mar 30, 2017

View reviewed changes

Remove unused instance variable

4d17add

erikjohnston merged commit 9cee0ce into develop Mar 31, 2017

erikjohnston deleted the erikj/cache_speed branch October 26, 2017 11:00

richvdh mentioned this pull request Jul 4, 2018

Consistently return a Deferred in the descriptor cache #3455

Closed

Uh oh!

Conversation

erikjohnston commented Mar 28, 2017

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!

richvdh commented Mar 30, 2017

Uh oh!

erikjohnston commented Mar 30, 2017

Uh oh!

erikjohnston commented Mar 30, 2017

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!

Choose a reason for hiding this comment

Uh oh!

erikjohnston commented Mar 30, 2017

Uh oh!

richvdh 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants