Issue 7751044: Queues (Closed)
|
Created:
11 years, 1 month ago by A. Jesse Jiryu Davis Modified:
11 years, 1 month ago Visibility:
Public. |
DescriptionQueues
Patch Set 1 #
Total comments: 24
Patch Set 2 : Responses to critique #
Total comments: 25
Patch Set 3 : Response to 2nd round of critique #
Total comments: 4
MessagesTotal messages: 21
For Issue 7: http://code.google.com/p/tulip/issues/detail?id=7 From my initial proposal: Tulip already has Lock, EventWaiter, Condition, and Semaphore. I propose it should have Queue, JoinableQueue, LifoQueue, and PriorityQueue. I offer to implement them. This patch is an initial implementation and a reasonably thorough test suite, ported from my package for Tornado, called "Toro" (https://github.com/ajdavis/toro).
Sign in to reply to this message.
On 2013/03/18 18:35:49, A. Jesse Jiryu Davis wrote: > For Issue 7: http://code.google.com/p/tulip/issues/detail?id=7 > > From my initial proposal: > > Tulip already has Lock, EventWaiter, Condition, and Semaphore. I propose it > should have Queue, JoinableQueue, LifoQueue, and PriorityQueue. I offer to > implement them. > > This patch is an initial implementation and a reasonably thorough test suite, > ported from my package for Tornado, called "Toro" > (https://github.com/ajdavis/toro). Great! i think we need queues. here is my thoughts: 1. Should we reuse locks? 2. Queue.put and Queue.get should be coroutines. 3. "block" parameter should be removed from put and get methods. create separate methods for non-blocking calls. 4. i dont think you need next_tick and _Waiter in case if for put() and get() are coroutines.
Sign in to reply to this message.
This is not a full review, just a bunch of notes on style and a few questions about possibly unnecessary functionality. I agree with all Nikolay's points. https://codereview.appspot.com/7751044/diff/1/tulip/queues.py File tulip/queues.py (right): https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode15 tulip/queues.py:15: def next_tick(event_loop, value): Doesn't this need a leading underscore? https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode23 tulip/queues.py:23: """Internal queue utility class""" Need a blank line below this. https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode25 tulip/queues.py:25: """Create a future with a timeout. If timeout is not None, it is an int I like to enforce the docstring style where, if there's more than one line, the format is """Summary sentence that fits on one line. <blank line> One or more paragraphs of text. """ https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode35 tulip/queues.py:35: self._event_loop.call_later( As an optimization, consider saving the handle(r) that this returns and calling cancel() on it when the result is set before the timeout is reached. https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode53 tulip/queues.py:53: """Create a queue object with a given maximum size. Don't use a verb to describe a class. https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode55 tulip/queues.py:55: If `maxsize` is ``None`` (the default) the queue size is unbounded. Please, no markup in docstrings. https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode63 tulip/queues.py:63: interrupted between calling :meth:`qsize` and doing an operation on the Ah, this is cool. :-) https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode68 tulip/queues.py:68: - `initial`: Optional sequence of initial items. How often is initial needed? The standard queue doesn't have this; I'm curious what the use case would be that makes it necessary to support this. https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode75 tulip/queues.py:75: raise ValueError("maxsize can't be negative") I recommend single quotes for all strings except docstrings. Maybe add repr(maxsize) to the error message -- the actual value may help the programmer understand the bug better. https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode127 tulip/queues.py:127: def get_maxsize(self): Maybe make this an internal method (_get_maxsize) so people won't call it instead of referencing q.maxsize ? https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode159 tulip/queues.py:159: def set_maxsize(self, maxsize): What's the use case for adjusting maxsize?
Sign in to reply to this message.
On 2013/03/18 20:10:22, GvR wrote: > This is not a full review, just a bunch of notes on style and a few questions > about possibly unnecessary functionality. I agree with all Nikolay's points. > > https://codereview.appspot.com/7751044/diff/1/tulip/queues.py > File tulip/queues.py (right): > > https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode15 > tulip/queues.py:15: def next_tick(event_loop, value): > Doesn't this need a leading underscore? > > https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode23 > tulip/queues.py:23: """Internal queue utility class""" > Need a blank line below this. > > https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode25 > tulip/queues.py:25: """Create a future with a timeout. If timeout is not None, > it is an int > I like to enforce the docstring style where, if there's more than one line, the > format is > > """Summary sentence that fits on one line. > <blank line> > One or more paragraphs of text. > """ > > https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode35 > tulip/queues.py:35: self._event_loop.call_later( > As an optimization, consider saving the handle(r) that this returns and calling > cancel() on it when the result is set before the timeout is reached. > > https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode53 > tulip/queues.py:53: """Create a queue object with a given maximum size. > Don't use a verb to describe a class. > > https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode55 > tulip/queues.py:55: If `maxsize` is ``None`` (the default) the queue size is > unbounded. > Please, no markup in docstrings. > > https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode63 > tulip/queues.py:63: interrupted between calling :meth:`qsize` and doing an > operation on the > Ah, this is cool. :-) > > https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode68 > tulip/queues.py:68: - `initial`: Optional sequence of initial items. > How often is initial needed? The standard queue doesn't have this; I'm curious > what the use case would be that makes it necessary to support this. > > https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode75 > tulip/queues.py:75: raise ValueError("maxsize can't be negative") > I recommend single quotes for all strings except docstrings. > > Maybe add repr(maxsize) to the error message -- the actual value may help the > programmer understand the bug better. > > https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode127 > tulip/queues.py:127: def get_maxsize(self): > Maybe make this an internal method (_get_maxsize) so people won't call it > instead of referencing q.maxsize ? > > https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode159 > tulip/queues.py:159: def set_maxsize(self, maxsize): > What's the use case for adjusting maxsize? Thanks guys, these are great critiques. I'll reply in detail tomorrow.
Sign in to reply to this message.
Responses to Guido are inline. Nikolay: > 1. Should we reuse locks? I tried that once with Toro and found it increased complexity substantially, instead of reducing it. > 2. Queue.put and Queue.get should be coroutines. Thanks, done. > 3. "block" parameter should be removed from put and get methods. > create separate methods for non-blocking calls. Done -- the new methods are put_nowait and get_nowait, following Gevent. > 4. i dont think you need next_tick and _Waiter > in case if for put() and get() are coroutines. I've removed next_tick; a simple yield works now. _Waiter is still useful for managing the timeout. https://codereview.appspot.com/7751044/diff/1/tulip/queues.py File tulip/queues.py (right): https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode15 tulip/queues.py:15: def next_tick(event_loop, value): On 2013/03/18 20:10:22, GvR wrote: > Doesn't this need a leading underscore? Done. https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode23 tulip/queues.py:23: """Internal queue utility class""" On 2013/03/18 20:10:22, GvR wrote: > Need a blank line below this. Done. https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode25 tulip/queues.py:25: """Create a future with a timeout. If timeout is not None, it is an int On 2013/03/18 20:10:22, GvR wrote: > I like to enforce the docstring style where, if there's more than one line, the > format is > > """Summary sentence that fits on one line. > <blank line> > One or more paragraphs of text. > """ Done. https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode35 tulip/queues.py:35: self._event_loop.call_later( On 2013/03/18 20:10:22, GvR wrote: > As an optimization, consider saving the handle(r) that this returns and calling > cancel() on it when the result is set before the timeout is reached. Thanks for the idea, done. https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode53 tulip/queues.py:53: """Create a queue object with a given maximum size. On 2013/03/18 20:10:22, GvR wrote: > Don't use a verb to describe a class. Done. https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode55 tulip/queues.py:55: If `maxsize` is ``None`` (the default) the queue size is unbounded. On 2013/03/18 20:10:22, GvR wrote: > Please, no markup in docstrings. How surprising! I'm accustomed to shops that encourage the exuberant use of RST in docstrings, for Sphinx. I'll remove it all. https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode68 tulip/queues.py:68: - `initial`: Optional sequence of initial items. On 2013/03/18 20:10:22, GvR wrote: > How often is initial needed? The standard queue doesn't have this; I'm curious > what the use case would be that makes it necessary to support this. I'll remove it, actually -- especially since it bypasses the usual code paths and seems bug-prone. I forgot to check it against maxsize.... https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode75 tulip/queues.py:75: raise ValueError("maxsize can't be negative") On 2013/03/18 20:10:22, GvR wrote: > I recommend single quotes for all strings except docstrings. > > Maybe add repr(maxsize) to the error message -- the actual value may help the > programmer understand the bug better. In this case there's a single-quote within the string so I advocate keeping the string double-quoted. I'll add the maxsize, though. https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode127 tulip/queues.py:127: def get_maxsize(self): On 2013/03/18 20:10:22, GvR wrote: > Maybe make this an internal method (_get_maxsize) so people won't call it > instead of referencing q.maxsize ? Done. https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode159 tulip/queues.py:159: def set_maxsize(self, maxsize): On 2013/03/18 20:10:22, GvR wrote: > What's the use case for adjusting maxsize? That's a very good question. Perhaps you're implementing a bounded message queue (like RabbitMQ) with Tulip, and you learn that total capacity has increased (e.g. someone added a backend server). Perhaps you want to increase the queue's maxsize in response, since you think your system can now work through a larger backlog in the same amount of time? I'm going to leave the feature in for a bit while we think about it, but I'm willing to remove it before we complete this review.
Sign in to reply to this message.
Second patch uploaded. This has: * Nikolay's idea to separate blocking and nonblocking methods * Nikolay's idea to make get() and put() coroutines * remove next_tick() * all Guido's style corrections * Guido's optimization, call cancel() on the timeout handler * remove 'initial' parameter as Guido recommended * make _Waiter a subclass of Future The only recommended changes I haven't made are: * remove _Waiter -- I think it's still useful * make maxsize immutable -- changing maxsize *could* be useful??
Sign in to reply to this message.
Good revision! Did you sign the PSF contrib agreement yet? I'm still pushing back on the two issues you left open... :-) https://codereview.appspot.com/7751044/diff/1/tulip/queues.py File tulip/queues.py (right): https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode75 tulip/queues.py:75: raise ValueError("maxsize can't be negative") On 2013/03/19 18:28:15, A. Jesse Jiryu Davis wrote: > On 2013/03/18 20:10:22, GvR wrote: > > I recommend single quotes for all strings except docstrings. > > > > Maybe add repr(maxsize) to the error message -- the actual value may help the > > programmer understand the bug better. > > In this case there's a single-quote within the string so I advocate keeping the > string double-quoted. I'll add the maxsize, though. D'oh. Good call. :-) https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode159 tulip/queues.py:159: def set_maxsize(self, maxsize): On 2013/03/19 18:28:15, A. Jesse Jiryu Davis wrote: > On 2013/03/18 20:10:22, GvR wrote: > > What's the use case for adjusting maxsize? > > That's a very good question. Perhaps you're implementing a bounded message queue > (like RabbitMQ) with Tulip, and you learn that total capacity has increased > (e.g. someone added a backend server). Perhaps you want to increase the queue's > maxsize in response, since you think your system can now work through a larger > backlog in the same amount of time? > > I'm going to leave the feature in for a bit while we think about it, but I'm > willing to remove it before we complete this review. Have you had an actual use case like that in the queue you implemented for Tornado? If so, then we should probably leave it in -- with a big comment explaining the use case. If it is more of theoretical importance, then I recommend leaving it out -- it's much easier to add functionality later than to delete functionality once it has been released. https://codereview.appspot.com/7751044/diff/8001/tests/queues_test.py File tests/queues_test.py (right): https://codereview.appspot.com/7751044/diff/8001/tests/queues_test.py#newcode4 tests/queues_test.py:4: from queue import Empty, Full # exceptions from standard lib's queue I'd rather see you import the module and then reference these with the module name as prefix (e.g. queue.Empty). Also, as a general comment, please start your sentences with a capital letter and terminate them with a period. https://codereview.appspot.com/7751044/diff/8001/tests/queues_test.py#newcode10 tests/queues_test.py:10: from tulip.tasks import coroutine, task, Task Here, also, I would rather say "from tulip import tasks". https://codereview.appspot.com/7751044/diff/8001/tests/queues_test.py#newcode13 tests/queues_test.py:13: # TODO: looks like Tulip needs a more convenient means of pausing You missed tulip.tasks.sleep(). :-) https://codereview.appspot.com/7751044/diff/8001/tests/queues_test.py#newcode247 tests/queues_test.py:247: return (yield from q.get(timeout=.1)) .1 -> 0.1 everywhere. https://codereview.appspot.com/7751044/diff/8001/tulip/queues.py File tulip/queues.py (right): https://codereview.appspot.com/7751044/diff/8001/tulip/queues.py#newcode15 tulip/queues.py:15: def _next_tick(event_loop, value): This is now unused, actually. https://codereview.appspot.com/7751044/diff/8001/tulip/queues.py#newcode32 tulip/queues.py:32: self.handler = self._event_loop.call_later( Sorry, we've just renamed handler to handle... https://codereview.appspot.com/7751044/diff/8001/tulip/queues.py#newcode59 tulip/queues.py:59: item is delivered. (This is unlike the standard Queue, where 0 means standard library Queue https://codereview.appspot.com/7751044/diff/8001/tulip/queues.py#newcode64 tulip/queues.py:64: interrupted between calling qsize() and doing an operation on the Queue. Maybe also add that you can change maxsize dynamically? https://codereview.appspot.com/7751044/diff/8001/tulip/queues.py#newcode70 tulip/queues.py:70: raise ValueError("maxsize can't be negative") You didn't add the maxsize value though. https://codereview.appspot.com/7751044/diff/8001/tulip/queues.py#newcode120 tulip/queues.py:120: """Number of items allowed in the queue. When I first saw this docstring I wondered why it was describing setting maxsize. I wonder if maybe it would be more appropriate to use property(_get_maxsize, _set_maxsize, None, <docstring>) below? https://codereview.appspot.com/7751044/diff/8001/tulip/queues.py#newcode189 tulip/queues.py:189: *before* the coroutine waiting for put() resumes. Is this relative timing an important property to guarantee? I could imagine all sorts of situations where it might be hard to guarantee this ordering, or where the user can't depend on it anyway because things are done a bit indirectly, and I don't have a good grasp on when it would be important to make this guarantee. https://codereview.appspot.com/7751044/diff/8001/tulip/queues.py#newcode207 tulip/queues.py:207: yield Hm... This use of a bare yield is actually undocumented and I'm not sure I want to keep it. Also, again, I'm curious why it is so important to yield here. I would rather have the fast path through put() not yield to the scheduler at all -- a scheduler roundtrip is kind of expensive. (Read my recent post about yield vs. yield-from for details.) https://codereview.appspot.com/7751044/diff/8001/tulip/queues.py#newcode216 tulip/queues.py:216: # The getter resumes now; the putter resumes on next tick Ditto.
Sign in to reply to this message.
i think _Waiter is quite useful on its own. lets rename it to something like TimedFuture or TimeoutFuture and make it part of public api.
Sign in to reply to this message.
On Tue, Mar 19, 2013 at 2:56 PM, <fafhrd91@gmail.com> wrote: > i think _Waiter is quite useful on its own. lets rename it to > something like TimedFuture or TimeoutFuture and make it part of public > api. > Agreed. But I was going to wait suggesting that refactoring until queues.py is committed. Possibly it should just be an extra parameter on the basic Future class? At some point in the past I had a Task class that also had a timeout parameter, so it might be useful for that too. > https://codereview.appspot.com/7751044/ > -- --Guido van Rossum (python.org/~guido)
Sign in to reply to this message.
On 2013/03/19 22:04:45, gvrpython wrote:
> On Tue, Mar 19, 2013 at 2:56 PM, <mailto:fafhrd91@gmail.com> wrote:
>
> > i think _Waiter is quite useful on its own. lets rename it to
> > something like TimedFuture or TimeoutFuture and make it part of public
> > api.
> >
>
> Agreed. But I was going to wait suggesting that refactoring until queues.py
> is committed. Possibly it should just be an extra parameter on the basic
> Future class? At some point in the past I had a Task class that also had a
> timeout parameter, so it might be useful for that too.
that was my initial idea, but i thought it would be too much :)
right now i have to write something like that, if i dont miss something:
def coroutine(...):
...
raise InternalError
done, pending = yield from tasks.wait([tasks.Task(coroutine())], timeout=...)
if done:
try:
res = done.pop().result()
except InternalError:
# process internal error
...
else:
# process timeout error
...
it could be more straightforward with timeout:
try:
res = yield from tasks.Tasks(coroutine(), timeout=...)
except TimeoutError:
# process timeout error
...
except InternalError:
# process internal error
...
Sign in to reply to this message.
On 2013/03/19 22:24:36, Nikolay Kim wrote: > On 2013/03/19 22:04:45, gvrpython wrote: > > On Tue, Mar 19, 2013 at 2:56 PM, <mailto:fafhrd91@gmail.com> wrote: > > > > > i think _Waiter is quite useful on its own. lets rename it to > > > something like TimedFuture or TimeoutFuture and make it part of public > > > api. > > > > > > > Agreed. But I was going to wait suggesting that refactoring until queues.py > > is committed. Possibly it should just be an extra parameter on the basic > > Future class? At some point in the past I had a Task class that also had a > > timeout parameter, so it might be useful for that too. > > that was my initial idea, but i thought it would be too much :) > > right now i have to write something like that, if i dont miss something: > > def coroutine(...): > ... > raise InternalError > > > done, pending = yield from tasks.wait([tasks.Task(coroutine())], timeout=...) > if done: > try: > res = done.pop().result() > except InternalError: > # process internal error > ... > else: > # process timeout error > ... > > > it could be more straightforward with timeout: > > try: > res = yield from tasks.Tasks(coroutine(), timeout=...) > except TimeoutError: > # process timeout error > ... > except InternalError: > # process internal error > ... Sounds good if you can pull this off. In the mean time let's encourage Jesse to get his current patch in shape (it's pretty close).
Sign in to reply to this message.
i've just added timeout support to Future.
Sign in to reply to this message.
(So, what Nikolay says, you can hg pull -u your repo, and then get rid of your _Waiter class, in favor of the regular Future. Should be fun!) On Wed, Mar 20, 2013 at 11:28 AM, <fafhrd91@gmail.com> wrote: > i've just added timeout support to Future. > > https://codereview.appspot.**com/7751044/<https://codereview.appspot.com/7751... > -- --Guido van Rossum (python.org/~guido)
Sign in to reply to this message.
> https://codereview.appspot.com/7751044/diff/8001/tulip/queues.py#newcode207 > tulip/queues.py:207: yield > Hm... This use of a bare yield is actually undocumented and I'm not sure I want > to keep it. Also, again, I'm curious why it is so important to yield here. I > would rather have the fast path through put() not yield to the scheduler at all > -- a scheduler roundtrip is kind of expensive. (Read my recent post about yield > vs. yield-from for details.) probably we should create tasks.call_next_tick() api. BTW "yield from [None]" should work as well :)
Sign in to reply to this message.
https://codereview.appspot.com/7751044/diff/1/tulip/queues.py File tulip/queues.py (right): https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode159 tulip/queues.py:159: def set_maxsize(self, maxsize): On 2013/03/19 20:55:15, GvR wrote: > On 2013/03/19 18:28:15, A. Jesse Jiryu Davis wrote: > > On 2013/03/18 20:10:22, GvR wrote: > > > What's the use case for adjusting maxsize? > > > > That's a very good question. Perhaps you're implementing a bounded message > queue > > (like RabbitMQ) with Tulip, and you learn that total capacity has increased > > (e.g. someone added a backend server). Perhaps you want to increase the > queue's > > maxsize in response, since you think your system can now work through a larger > > backlog in the same amount of time? > > > > I'm going to leave the feature in for a bit while we think about it, but I'm > > willing to remove it before we complete this review. > > Have you had an actual use case like that in the queue you implemented for > Tornado? If so, then we should probably leave it in -- with a big comment > explaining the use case. If it is more of theoretical importance, then I > recommend leaving it out -- it's much easier to add functionality later than to > delete functionality once it has been released. OK, I'm convinced and I'll remove it. This code can be put unmodified in a subclass outside the standard library if anyone needs it. I'll change it from a property to a method, also: queue.maxsize(). That's consistent with qsize(), empty(), etc. Do you agree or should they all be properties? https://codereview.appspot.com/7751044/diff/8001/tests/queues_test.py File tests/queues_test.py (right): https://codereview.appspot.com/7751044/diff/8001/tests/queues_test.py#newcode4 tests/queues_test.py:4: from queue import Empty, Full # exceptions from standard lib's queue On 2013/03/19 20:55:15, GvR wrote: > I'd rather see you import the module and then reference these with the module > name as prefix (e.g. queue.Empty). > > Also, as a general comment, please start your sentences with a capital letter > and terminate them with a period. Done. https://codereview.appspot.com/7751044/diff/8001/tests/queues_test.py#newcode10 tests/queues_test.py:10: from tulip.tasks import coroutine, task, Task On 2013/03/19 20:55:15, GvR wrote: > Here, also, I would rather say "from tulip import tasks". Done. https://codereview.appspot.com/7751044/diff/8001/tests/queues_test.py#newcode13 tests/queues_test.py:13: # TODO: looks like Tulip needs a more convenient means of pausing On 2013/03/19 20:55:15, GvR wrote: > You missed tulip.tasks.sleep(). :-) D'oh. https://codereview.appspot.com/7751044/diff/8001/tests/queues_test.py#newcode247 tests/queues_test.py:247: return (yield from q.get(timeout=.1)) On 2013/03/19 20:55:15, GvR wrote: > .1 -> 0.1 everywhere. Done. https://codereview.appspot.com/7751044/diff/8001/tulip/queues.py File tulip/queues.py (right): https://codereview.appspot.com/7751044/diff/8001/tulip/queues.py#newcode15 tulip/queues.py:15: def _next_tick(event_loop, value): On 2013/03/19 20:55:15, GvR wrote: > This is now unused, actually. Done. https://codereview.appspot.com/7751044/diff/8001/tulip/queues.py#newcode59 tulip/queues.py:59: item is delivered. (This is unlike the standard Queue, where 0 means On 2013/03/19 20:55:15, GvR wrote: > standard library Queue Done. https://codereview.appspot.com/7751044/diff/8001/tulip/queues.py#newcode70 tulip/queues.py:70: raise ValueError("maxsize can't be negative") On 2013/03/19 20:55:15, GvR wrote: > You didn't add the maxsize value though. Oops. But now irrelevant -- in my next patch I won't validate maxsize at all, same as stdlib doesn't validate it. https://codereview.appspot.com/7751044/diff/8001/tulip/queues.py#newcode120 tulip/queues.py:120: """Number of items allowed in the queue. On 2013/03/19 20:55:15, GvR wrote: > When I first saw this docstring I wondered why it was describing setting > maxsize. I wonder if maybe it would be more appropriate to use > > property(_get_maxsize, _set_maxsize, None, <docstring>) > > below? Done. https://codereview.appspot.com/7751044/diff/8001/tulip/queues.py#newcode189 tulip/queues.py:189: *before* the coroutine waiting for put() resumes. On 2013/03/19 20:55:15, GvR wrote: > Is this relative timing an important property to guarantee? I could imagine all > sorts of situations where it might be hard to guarantee this ordering, or where > the user can't depend on it anyway because things are done a bit indirectly, and > I don't have a good grasp on when it would be important to make this guarantee. Honestly, I don't have a good grasp either. This guarantee is made by Gevent's queues, at least in the circumstances Gevent's unittests cover, so I copied the guarantee in Toro and now in Tulip. I was assuming Denis Bilenko had a good reason for it, but I'm reconsidering. Note that the upcoming Gevent 1.0 release will change the behavior of "Queue(0)": it will be an unlimited queue, the same as "Queue(None)". This is the same API is the standard library Queue. The "channel" behavior of Gevent's old Queue(0) will move into a separate Channel class in Gevent 1.0. I've asked in the Gevent mailing list what the point of Channel is, and why coroutines awakened by put() or get() must resume *before* the calling coroutine. In any case, in my next patch I'll just make tulip's Queue act less like Gevent's and more like the standard library's: 0 means unbounded, and the ordering guarantee is dropped. https://codereview.appspot.com/7751044/diff/8001/tulip/queues.py#newcode207 tulip/queues.py:207: yield On 2013/03/19 20:55:15, GvR wrote: > Hm... This use of a bare yield is actually undocumented and I'm not sure I want > to keep it. Also, again, I'm curious why it is so important to yield here. I > would rather have the fast path through put() not yield to the scheduler at all > -- a scheduler roundtrip is kind of expensive. (Read my recent post about yield > vs. yield-from for details.) Where can I see that post? Anyway, I'm going to remove this.
Sign in to reply to this message.
This patch might be ready to merge (no outstanding issues I know of). * _Waiter is gone * maxsize() is now immutable, now a method instead of property * Queue is now more like the standard library Queue and less like Gevent's: Queue(0) now means "unbounded Queue" whereas in Gevent "Queue(0)" means "channel." Queue(0) is now the default, rather than Queue(None). Queue(None) no longer means anything. * put() now continues if possible, instead of immediately handing control to any coroutine that's paused at "yield from q.get()" * same for get(): it now continues instead of handing control to a coroutine yielding from put() * no more raw yields I *have* signed the contributor agreement.
Sign in to reply to this message.
On 2013/03/20 22:27:00, A. Jesse Jiryu Davis wrote: > This patch might be ready to merge (no outstanding issues I know of). > > * _Waiter is gone > > * maxsize() is now immutable, now a method instead of property > > * Queue is now more like the standard library Queue and less like Gevent's: > Queue(0) now means "unbounded Queue" whereas in Gevent "Queue(0)" means > "channel." Queue(0) is now the default, rather than Queue(None). Queue(None) no > longer means anything. > > * put() now continues if possible, instead of immediately handing control to any > coroutine that's paused at "yield from q.get()" > > * same for get(): it now continues instead of handing control to a coroutine > yielding from put() > > * no more raw yields > > I *have* signed the contributor agreement. i can't apply latest patch. seems you did some local commits.
Sign in to reply to this message.
Just one thing left, really -- maxsize needs to be a property. Feel free to commit after fixing this (and the task->coroutine thing in the tests), doing hg pull -u (and possibly fixing conflicts), and running all tests once more. (Nikolay: the problems with applying the patch are probably because Jesse is way behind compared to the tip.) https://codereview.appspot.com/7751044/diff/8001/tulip/queues.py File tulip/queues.py (right): https://codereview.appspot.com/7751044/diff/8001/tulip/queues.py#newcode189 tulip/queues.py:189: *before* the coroutine waiting for put() resumes. On 2013/03/20 22:23:46, A. Jesse Jiryu Davis wrote: > On 2013/03/19 20:55:15, GvR wrote: > > Is this relative timing an important property to guarantee? I could imagine > all > > sorts of situations where it might be hard to guarantee this ordering, or > where > > the user can't depend on it anyway because things are done a bit indirectly, > and > > I don't have a good grasp on when it would be important to make this > guarantee. > > Honestly, I don't have a good grasp either. This guarantee is made by Gevent's > queues, at least in the circumstances Gevent's unittests cover, so I copied the > guarantee in Toro and now in Tulip. I was assuming Denis Bilenko had a good > reason for it, but I'm reconsidering. > > Note that the upcoming Gevent 1.0 release will change the behavior of > "Queue(0)": it will be an unlimited queue, the same as "Queue(None)". This is > the same API is the standard library Queue. The "channel" behavior of Gevent's > old Queue(0) will move into a separate Channel class in Gevent 1.0. > > I've asked in the Gevent mailing list what the point of Channel is, and why > coroutines awakened by put() or get() must resume *before* the calling > coroutine. > > In any case, in my next patch I'll just make tulip's Queue act less like > Gevent's and more like the standard library's: 0 means unbounded, and the > ordering guarantee is dropped. Thanks! You can add a Channel class later. https://codereview.appspot.com/7751044/diff/8001/tulip/queues.py#newcode207 tulip/queues.py:207: yield On 2013/03/20 22:23:46, A. Jesse Jiryu Davis wrote: > On 2013/03/19 20:55:15, GvR wrote: > > Hm... This use of a bare yield is actually undocumented and I'm not sure I > want > > to keep it. Also, again, I'm curious why it is so important to yield here. I > > would rather have the fast path through put() not yield to the scheduler at > all > > -- a scheduler roundtrip is kind of expensive. (Read my recent post about > yield > > vs. yield-from for details.) > > Where can I see that post? Anyway, I'm going to remove this. In the python-tulip Google Group. https://codereview.appspot.com/7751044/diff/23001/tests/queues_test.py File tests/queues_test.py (right): https://codereview.appspot.com/7751044/diff/23001/tests/queues_test.py#newcod... tests/queues_test.py:126: @tasks.task You can now make this use coroutine instead of task. https://codereview.appspot.com/7751044/diff/23001/tulip/queues.py File tulip/queues.py (right): https://codereview.appspot.com/7751044/diff/23001/tulip/queues.py#newcode78 tulip/queues.py:78: def maxsize(self): This needs to be a property to be compatible with the stdlib -- it has maxsize as an attribute, but qsize() and empty() as methods (because the latter need to acquire the lock).
Sign in to reply to this message.
https://codereview.appspot.com/7751044/diff/23001/tests/queues_test.py File tests/queues_test.py (right): https://codereview.appspot.com/7751044/diff/23001/tests/queues_test.py#newcod... tests/queues_test.py:126: @tasks.task On 2013/03/21 00:16:48, GvR wrote: > You can now make this use coroutine instead of task. Cool; I'll use coroutine throughout the test suite, then. When I need a task I'll do task.Task() at the call site, rather than the definition site. https://codereview.appspot.com/7751044/diff/23001/tulip/queues.py File tulip/queues.py (right): https://codereview.appspot.com/7751044/diff/23001/tulip/queues.py#newcode78 tulip/queues.py:78: def maxsize(self): On 2013/03/21 00:16:48, GvR wrote: > This needs to be a property to be compatible with the stdlib -- it has maxsize > as an attribute, but qsize() and empty() as methods (because the latter need to > acquire the lock). Done.
Sign in to reply to this message.
On 2013/03/21 00:50:26, A. Jesse Jiryu Davis wrote: > https://codereview.appspot.com/7751044/diff/23001/tests/queues_test.py > File tests/queues_test.py (right): > > https://codereview.appspot.com/7751044/diff/23001/tests/queues_test.py#newcod... > tests/queues_test.py:126: @tasks.task > On 2013/03/21 00:16:48, GvR wrote: > > You can now make this use coroutine instead of task. > > Cool; I'll use coroutine throughout the test suite, then. When I need a task > I'll do task.Task() at the call site, rather than the definition site. > > https://codereview.appspot.com/7751044/diff/23001/tulip/queues.py > File tulip/queues.py (right): > > https://codereview.appspot.com/7751044/diff/23001/tulip/queues.py#newcode78 > tulip/queues.py:78: def maxsize(self): > On 2013/03/21 00:16:48, GvR wrote: > > This needs to be a property to be compatible with the stdlib -- it has maxsize > > as an attribute, but qsize() and empty() as methods (because the latter need > to > > acquire the lock). > > Done. Thanks Guido and Nikolay, this has been an insanely great code review.
Sign in to reply to this message.
Message was sent while issue was closed.
On 2013/03/21 01:02:08, A. Jesse Jiryu Davis wrote: > Thanks Guido and Nikolay, this has been an insanely great code review. Thanks Jesse for the contribution!! I hope you will make more. (Did you get the email about using parameter-less super()?)
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
