-
Notifications
You must be signed in to change notification settings - Fork 29
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
Refactored Synchroniser to use simpler self._futures construct #49
Refactored Synchroniser to use simpler self._futures construct #49
Conversation
# (spec.BasicGetOK, spec.BasicGetEmpty) | ||
for other_method in bound_methods: | ||
if other_method != method: | ||
self._futures[other_method].remove((fut, bound_methods)) |
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.
This is potentially expensive. deque.remove
has to search through the deque for the item it's looking for, and removing it means copying a potentially large number of other items into place. In other words remove
is O(n), whereas the OrderedDict
version was amortised O(1).
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.
Ok, I think I know how to fix this right away.
Fixed |
GH won't let me merge it because there are conflicts. Please could you resolve them? |
5924c79
to
bf2a93b
Compare
done |
bump, if no more problems, merge this one pls. |
Refactored Synchroniser to use simpler self._futures construct
Will ease work on #34, as it remembers the exception, the synchroniser was killed with to raise them later.