This repository has been archived by the owner on Apr 26, 2024. It is now read-only.
Fix bug where state cache used lots of memory #3005
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The state cache bases its size on the sum of the size of entries. The size of the entry is calculated once on insertion, so it is important that the size of entries does not change. The DictionaryCache modified the entries size, which caused the state cache to incorrectly think it was smaller than it actually was.
erikjohnston
commented
Mar 15, 2018
synapse/util/caches/lrucache.py
Outdated
| @@ -154,14 +154,14 @@ def cache_get(key, default=None, callbacks=[]): | |||
| def cache_set(key, value, callbacks=[]): | |||
| node = cache.get(key, None) | |||
| if node is not None: | |||
| if value != node.value: | |||
| if node.callbacks and value != node.value: | |||
There was a problem hiding this comment.
Oh, this was just a cheeky thing to only do the != check if we do actually have node.callbacks
There was a problem hiding this comment.
isn't it outweighed by doing size_callback even when it doesn't change?
richvdh
approved these changes
Mar 15, 2018
synapse/util/caches/lrucache.py
Outdated
| @@ -154,14 +154,14 @@ def cache_get(key, default=None, callbacks=[]): | |||
| def cache_set(key, value, callbacks=[]): | |||
| node = cache.get(key, None) | |||
| if node is not None: | |||
| if value != node.value: | |||
| if node.callbacks and value != node.value: | |||
synapse/util/caches/lrucache.py
Outdated
| @@ -154,14 +154,14 @@ def cache_get(key, default=None, callbacks=[]): | |||
| def cache_set(key, value, callbacks=[]): | |||
| node = cache.get(key, None) | |||
| if node is not None: | |||
| if value != node.value: | |||
| if node.callbacks and value != node.value: | |||
There was a problem hiding this comment.
isn't it outweighed by doing size_callback even when it doesn't change?
| # We pop and reinsert as we need to tell the cache the size may have | ||
| # changed | ||
|
|
||
| entry = self.cache.pop(key, DictionaryEntry(False, set(), {})) |
There was a problem hiding this comment.
not entirely relevant here, but I wonder if it would be better to do
entry = self.cache.pop(key, None)
if entry is None:
entry = DictionaryEntry(False, set(), {})
to avoid the object creation overhead?
|
please can we merge this and cut a new RC before any more folks on 0.27-rc1 have explosions? |
|
(The PEP8 error appears spurious and unrelated to this PR) |
krombel
added a commit
to krombel/synapse
that referenced
this pull request
Mar 22, 2018
Changes in synapse v0.27.0-rc2 (2018-03-19) =========================================== Pulls in v0.26.1 Bug fixes: * Fix bug introduced in v0.27.0-rc1 that causes much increased memory usage in state cache (PR matrix-org#3005) Changes in synapse v0.26.1 (2018-03-15) ======================================= Bug fixes: * Fix bug where an invalid event caused server to stop functioning correctly, due to parsing and serializing bugs in ujson library (PR matrix-org#3008)
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The state cache bases its size on the sum of the size of entries. The
size of the entry is calculated once on insertion, so it is important
that the size of entries does not change.
The DictionaryCache modified the entries size, which caused the state
cache to incorrectly think it was smaller than it actually was.