Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Optimisation in DeferredCache.set #8593

Merged
merged 2 commits into from
Oct 21, 2020
Merged

Optimisation in DeferredCache.set #8593

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2b3af01
optimise DeferredCache.set
richvdh Oct 16, 2020
c13820b
fix failure case
richvdh Oct 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/8593.misc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Minor optimisations in caching code.
18 changes: 15 additions & 3 deletions synapse/util/caches/deferred_cache.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
from prometheus_client import Gauge

from twisted.internet import defer
from twisted.python import failure

from synapse.util.async_helpers import ObservableDeferred
from synapse.util.caches.lrucache import LruCache
Expand Down Expand Up @@ -214,16 +215,27 @@ def set(

callbacks = [callback] if callback else []
self.check_thread()
observable = ObservableDeferred(value, consumeErrors=True)
observer = observable.observe()
entry = CacheEntry(deferred=observable, callbacks=callbacks)

existing_entry = self._pending_deferred_cache.pop(key, None)
if existing_entry:
existing_entry.invalidate()

# XXX: why don't we invalidate the entry in `self.cache` yet?

# we can save a whole load of effort if the deferred is ready.
if value.called:
result = value.result
if not isinstance(result, failure.Failure):
self.cache.set(key, result, callbacks)
return value
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really sure it matters, but switching from returning ObservableDeferred.observe() to value means that the result of this Deferred could now be eaten in the callback chain where previously it wouldn't be. Perhaps this should be return defer.succeed(value)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it matters if it gets eaten further down the callback chain? we've got the result and done everything we need with it?

Perhaps this should be return defer.succeed(value)?

value is itself a Deferred, so possibly defer.succeed(value.result), but I'm not sure it's necessary.

(Tests might be fixed now)

Copy link
Member

@clokep clokep Oct 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to come up with a situation in which it would matter and I'm struggling to find any. It might matter if the caller is setting the cache and doing other things with the original deferred, but I'm not sure that would make much sense. 🤷


# otherwise, we'll add an entry to the _pending_deferred_cache for now,
# and add callbacks to add it to the cache properly later.

observable = ObservableDeferred(value, consumeErrors=True)
observer = observable.observe()
entry = CacheEntry(deferred=observable, callbacks=callbacks)

self._pending_deferred_cache[key] = entry

def compare_and_pop():
Expand Down