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

[WIP] Issue198 #209

Merged
merged 1 commit into from Jan 27, 2017
Merged

[WIP] Issue198 #209

merged 1 commit into from Jan 27, 2017

Conversation

apollo13
Copy link
Contributor

@apollo13 apollo13 commented Jan 22, 2017

If this works out on PyPY the patch needs cleanups. I am also imagining
the following: Always use a deque and add a flag to the manager if
priority events should be supported. If not we don't have to use the
heap queue at all which might result in performance improvements.

@apollo13 apollo13 added this to the 3.3 milestone Jan 22, 2017
@apollo13 apollo13 self-assigned this Jan 22, 2017
@codecov-io
Copy link

codecov-io commented Jan 22, 2017

Current coverage is 77.97% (diff: 100%)

Merging #209 into master will increase coverage by 0.91%

@@             master       #209   diff @@
==========================================
  Files            80         80          
  Lines          6637       6657    +20   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           5115       5191    +76   
+ Misses         1522       1466    -56   
  Partials          0          0          

Powered by Codecov. Last update 25442fc...7443fb9

@@ -948,7 +959,7 @@ def run(self, socket=None):
self.fire(started(self))

try:
while self.running or self._queue:
while self.running or len(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we used this explicit check because if a component would overwrite __len__ this would have implications. See #51 for a similar issue.

Copy link
Member

@prologic prologic left a comment

Choose a reason for hiding this comment

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

Is it worth abstracting out the internal queue into a class that does what we want here? We could make things much more clear then right?

class EventQueue(object):

    def __init__(...):
        self._queue = deque()
        self._priority_queue = []

@@ -219,7 +221,7 @@ def __repr__(self):

channel = "/{0:s}".format(str(getattr(self, "channel", "")))

q = len(self._queue)
q = len(self)
Copy link
Member

Choose a reason for hiding this comment

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

For the same reason we should be explicit here in case someone overrides __len__ on their components :/ People do silly things :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you two are absolutely right -- a custom object is probably the "most compatible"

@prologic
Copy link
Member

Let's also get @spaceone view on this?

@spaceone
Copy link
Contributor

@prologic I had a look at the changes which from the diff look okay. But I need to dig further into this.

Copy link
Contributor

@spaceone spaceone left a comment

Choose a reason for hiding this comment

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

LGTM. Just some small not so important questions.

@@ -126,6 +127,41 @@ def __init__(self, timeout):
self.tick_handler = None


class EventQueue(object):
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe private _EventQueue?

self._flush_batch = 0

def __len__(self):
return len(self._queue) + len(self._priority_queue)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe implement also __nonzero__/__bool__?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Documentation for __nonzero__/__bool__:

When this method is not defined, __len__() is called, if it is defined, and the object is considered true if its result is nonzero.

def __len__(self):
return len(self._queue) + len(self._priority_queue)

def drainFrom(self, other_queue):
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to keep the camelCase thing for new implementations? PEP8 wants lower_underscoe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am fine with either, a mixed codebase looks somewhat weird though :D

@@ -126,6 +127,41 @@ def __init__(self, timeout):
self.tick_handler = None


class EventQueue(object):
Copy link
Contributor

Choose a reason for hiding this comment

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

Add __slots__ = ('_queue', '_priority_queue', '_counter', '_flush_batch')?


def dispatchEvents(self, dispatcher):
if self._flush_batch == 0:
# XXX: Might be faster to use heapify instead of pop +
Copy link
Contributor

Choose a reason for hiding this comment

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

I personally like s/XXX/FIXME/ more.

@@ -909,7 +930,7 @@ def tick(self, timeout=-1):
if self._running:
self.fire(generate_events(self._lock, timeout), "*")

self._queue and self.flush()
len(self._queue) and self.flush()
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe split into 2 lines? Why should we use this syntax? Performance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No reason imo, splitting it makes the intent certainly cleaner

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants