-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Speed up fetching device lists changes in sync. #7423
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Speed up fetching device lists changes when handling `/sync` requests. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,12 +14,13 @@ | |
# limitations under the License. | ||
|
||
import logging | ||
from typing import Dict, Iterable, List, Mapping, Optional, Set | ||
from typing import Dict, FrozenSet, List, Mapping, Optional, Set, Union | ||
|
||
from six import integer_types | ||
|
||
from sortedcontainers import SortedDict | ||
|
||
from synapse.types import Collection | ||
from synapse.util import caches | ||
|
||
logger = logging.getLogger(__name__) | ||
|
@@ -85,16 +86,26 @@ def has_entity_changed(self, entity: EntityType, stream_pos: int) -> bool: | |
return False | ||
|
||
def get_entities_changed( | ||
self, entities: Iterable[EntityType], stream_pos: int | ||
) -> Set[EntityType]: | ||
self, entities: Collection[EntityType], stream_pos: int | ||
) -> Union[Set[EntityType], FrozenSet[EntityType]]: | ||
""" | ||
Returns subset of entities that have had new things since the given | ||
position. Entities unknown to the cache will be returned. If the | ||
position is too old it will just return the given list. | ||
""" | ||
changed_entities = self.get_all_entities_changed(stream_pos) | ||
if changed_entities is not None: | ||
result = set(changed_entities).intersection(entities) | ||
# We now do an intersection, trying to do so in the most efficient | ||
# way possible (some of these sets are *large*). First check in the | ||
# given iterable is already set that we can reuse, otherwise we | ||
# create a set of the *smallest* of the two iterables and call | ||
# `intersection(..)` on it (this can be twice as fast as the reverse). | ||
Comment on lines
+101
to
+102
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. https://wiki.python.org/moin/TimeComplexity#set has some stuff to say about this:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well that is interesting, I did:
I'm slightly surprised that they're symmetric when (I'm going to wait for you to point out my embarrassing typo now....) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks sane to me. The only thing I can think of is that this is partially bound by the creation of the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, that'd make sense if they ignored the time requirements for creating the set There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, so it'd be interesting to see: >>> sa = set(a)
>>> sb = set(b)
>>> %timeit sa.intersection(b)
>>> %timeit sb.intersection(a) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I looked at this a bit more and I think your conclusion is sane, but the gist of my exploration is that converting things to sets is expensive. 👍 |
||
if isinstance(entities, (set, frozenset)): | ||
result = entities.intersection(changed_entities) | ||
elif len(changed_entities) < len(entities): | ||
result = set(changed_entities).intersection(entities) | ||
else: | ||
result = set(entities).intersection(changed_entities) | ||
self.metrics.inc_hits() | ||
else: | ||
result = set(entities) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've checked all the callers and this matches their types fwiw