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

Fix LruCache callback deduplication #6213

Merged
merged 8 commits into from Nov 7, 2019

Conversation

@V02460
Copy link
Contributor

V02460 commented Oct 17, 2019

The PR fixes the deduplication issues for memoizing caches. It makes all identical _CacheContext objects share the same invalidate function. This is required for proper deduplication of invalidate callbacks in LruCache. LruCache is used for the @cached function decorator. Fixes #6200.

LruCache relies on the equality of callback functions for deduplication. This is in contrast to what was annotated in _CacheContext which claims that LruCache relies on the equality of _CacheContext objects itself.

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file
  • Pull request includes a sign off
Signed-off-by: Kai A. Hiller <KaiAlexHiller@web.de>
@V02460 V02460 force-pushed the V02460:cache_context branch from d427dbf to a4f062e Oct 17, 2019
Kai A. Hiller added 2 commits Oct 17, 2019
Signed-off-by: Kai A. Hiller <KaiAlexHiller@web.de>
Signed-off-by: Kai A. Hiller <KaiAlexHiller@web.de>
@V02460 V02460 mentioned this pull request Oct 24, 2019
@hawkowl hawkowl requested a review from matrix-org/synapse-core Oct 24, 2019
@richvdh

This comment has been minimized.

Copy link
Member

richvdh commented Oct 29, 2019

Copy link
Member

richvdh left a comment

thank you very much for the contribution!

This is in contrast to what was annotated in _CacheContext which claims that LruCache relies on the equality of _CacheContext objects itself.

well, it was kind of true, in that the equality of the invalidate methods depended on the equality of the _CacheContext objects, though I agree it wasn't very clear.

Anyway: I think this might be more intuitively solved by ensuring that we don't create duplicate _CacheContext objects, so:

  • leave _CacheContext as it was
  • create in CacheDescriptor.__get__, do cache_contexts = {}
  • at line 433, do kwargs["cache_context"] = cache_contexts.setdefault(cache_key, _CacheContext(cache, cache_key))

wdyt?

@richvdh richvdh added this to In progress in Homeserver Task Board via automation Oct 29, 2019
@richvdh richvdh self-assigned this Oct 29, 2019
@richvdh richvdh moved this from In progress to Community PRs in Homeserver Task Board Oct 29, 2019
@V02460

This comment has been minimized.

Copy link
Contributor Author

V02460 commented Oct 31, 2019

Ensuring _CacheContext objects are not duplicated seems saner to me as well, right.

I'd argue that it's not CacheDescriptor's responsibility to manage caching of _CacheContext instances. Because of this and to not clutter CacheDescriptor even more, it might be better to put that code into a get_instance factory method of _CacheContext instead.

I think we still need to use a WeakValueDictionary instead of a normal dict as you suggested. For every new argument of a method call a _CacheContext object is created, so these become many and really should be released when no longer used.

@richvdh

This comment has been minimized.

Copy link
Member

richvdh commented Oct 31, 2019

I'd argue that it's not CacheDescriptor's responsibility to manage caching of _CacheContext instances. Because of this and to not clutter CacheDescriptor even more, it might be better to put that code into a get_instance factory method of _CacheContext instead.

ok, that sounds reasonable.

I think we still need to use a WeakValueDictionary instead of a normal dict as you suggested. For every new argument of a method call a _CacheContext object is created, so these become many and really should be released when no longer used.

I think you're right. I hadn't quite thought that part through!

Signed-off-by: Kai A. Hiller <KaiAlexHiller@web.de>
@V02460 V02460 force-pushed the V02460:cache_context branch from 36bc8a9 to 8d7211d Oct 31, 2019
@richvdh richvdh self-requested a review Nov 4, 2019
@richvdh
richvdh approved these changes Nov 6, 2019
Copy link
Member

richvdh left a comment

lgtm. I've merged latest develop into the branch to try to get the CI to sort itself out.

@richvdh richvdh self-requested a review Nov 6, 2019
Kai A. Hiller Kai A. Hiller
@V02460 V02460 force-pushed the V02460:cache_context branch from ce3785e to 94be60f Nov 6, 2019
@richvdh

This comment has been minimized.

Copy link
Member

richvdh commented Nov 7, 2019

🎉

@richvdh richvdh merged commit affcc2c into matrix-org:develop Nov 7, 2019
20 checks passed
20 checks passed
buildkite/synapse Build #5452 passed (21 minutes, 24 seconds)
Details
buildkite/synapse/check-sample-config Passed (1 minute, 29 seconds)
Details
buildkite/synapse/check-style Passed (1 minute, 44 seconds)
Details
buildkite/synapse/isort Passed (16 seconds)
Details
buildkite/synapse/mypy Passed (26 seconds)
Details
buildkite/synapse/newspaper-newsfile Passed (15 seconds)
Details
buildkite/synapse/packaging Passed (45 seconds)
Details
buildkite/synapse/pipeline Passed (3 seconds)
Details
buildkite/synapse/python-3-dot-5-slash-postgres-9-dot-5 Passed (17 minutes, 43 seconds)
Details
buildkite/synapse/python-3-dot-5-slash-sqlite Passed (6 minutes, 36 seconds)
Details
buildkite/synapse/python-3-dot-5-slash-sqlite-slash-old-deps Passed (8 minutes, 49 seconds)
Details
buildkite/synapse/python-3-dot-6-slash-sqlite Passed (7 minutes, 3 seconds)
Details
buildkite/synapse/python-3-dot-7-slash-postgres-11 Passed (17 minutes, 53 seconds)
Details
buildkite/synapse/python-3-dot-7-slash-postgres-9-dot-5 Passed (17 minutes, 35 seconds)
Details
buildkite/synapse/python-3-dot-7-slash-sqlite Passed (6 minutes, 43 seconds)
Details
buildkite/synapse/synapse-port-db-slash-python-3-dot-5-slash-postgres-9-dot-5 Passed (1 minute, 52 seconds)
Details
buildkite/synapse/synapse-port-db-slash-python-3-dot-7-slash-postgres-11 Passed (1 minute, 51 seconds)
Details
buildkite/synapse/sytest-python-3-dot-5-slash-postgres-9-dot-6-slash-monolith Passed (15 minutes, 53 seconds)
Details
buildkite/synapse/sytest-python-3-dot-5-slash-postgres-9-dot-6-slash-workers Passed (13 minutes, 42 seconds)
Details
buildkite/synapse/sytest-python-3-dot-5-slash-sqlite-slash-monolith Passed (13 minutes, 35 seconds)
Details
Homeserver Task Board automation moved this from Community PRs to Done Nov 7, 2019
@richvdh

This comment has been minimized.

Copy link
Member

richvdh commented Nov 7, 2019

thank you very much for contributing this and for working through the pain from the CI!

@auscompgeek

This comment has been minimized.

Copy link

auscompgeek commented Nov 7, 2019

FYI you didn't have to turn the function annotations into comments - that's valid Python 3 syntax.

@V02460

This comment has been minimized.

Copy link
Contributor Author

V02460 commented Nov 8, 2019

Oh thank you, I didn't know function annotation syntax was available earlier (PEP 3107, Python 3.0) than variable annotation syntax (PEP 526, Python 3.6).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
3 participants
You can’t perform that action at this time.