Add desmod.pool.Pool for modeling pool of resources #16
Conversation
vagvaz
force-pushed
the
master
branch
2 times, most recently
from
286f77d
to
cba4d3c
Compare
desmod/pool.py
Outdated
| def _trigger_get(self, _=None): | ||
| if self._getters and self.level: | ||
| for index, get_ev in enumerate(self._getters): | ||
| get_ev = self._getters[index] |
There was a problem hiding this comment.
Why reassign get_ev? It already exists as a loop variable?
desmod/pool.py
Outdated
| get_ev.succeed(item) | ||
| if self._get_hook: | ||
| self._get_hook() | ||
| break |
There was a problem hiding this comment.
The control flow in _trigger_get() is such that it is possible for the getter at the head of the line (_getters) can be starved. This can happen if the first getter has a large amount and subsequent getters have small amounts. The issue is that the _getters is not treated in a FIFO manner. This is inconsistent with simpy.Container which always fulfills the first getter before fulfilling any subsequent getters.
Another difference with simpy.Container is that _trigger_get() only fulfills one getter instead of fulfilling getters (in FIFO order) until level is below the next getter's amount.
I suggest that we want Pool's semantics to be aligned with simpy.Container unless there is a compelling reason I am unaware of.
desmod/pool.py
Outdated
| put_ev = self._putters.pop(0) | ||
| put_ev.succeed() | ||
| self._add_items(put_ev.amount) | ||
| self._trigger_when_new() |
There was a problem hiding this comment.
The when_new waiters will be triggered unconditionally even if the putter's amount is zero. Although somewhat pathological for a putter to have amount=0, it might be best to nonetheless protect _trigger_when_new() with a test of level.
desmod/pool.py
Outdated
| """Simulation pool of arbitrary items. | ||
|
|
||
| `Pool` is similar to :class:`simpy.resources.Container`. | ||
| It provides a simulation-aware container for managing a pool of objects |
There was a problem hiding this comment.
Nit, simpy.Container and thus Pool do not necessarily model discrete objects. An important use case is to model continuous quantities (i.e. with a float level). This doc string should be clarified.
desmod/pool.py
Outdated
| @property | ||
| def size(self): | ||
| """Number of items in pool.""" | ||
| return self.level |
There was a problem hiding this comment.
It's not clear why we need/want this size alias for level.
desmod/probe.py
Outdated
| @@ -124,3 +130,19 @@ def hook(): | |||
| callback(queue.remaining) | |||
|
|
|||
| queue._put_hook = queue._get_hook = hook | |||
|
|
|||
|
|
|||
| def _attach_pool_size(pool, callbacks): | |||
There was a problem hiding this comment.
Naming: _attach_pool_level()?
desmod/pool.py
Outdated
|
|
||
| def _remove_items(self, item_count=1): | ||
| self.level -= item_count | ||
| return item_count |
There was a problem hiding this comment.
These _add_items() and _remove_items() methods only have one call site each and do a trivial operation. I'd suggest moving these level increments and decrements inline.
vagvaz
force-pushed
the
master
branch
2 times, most recently
from
075d9a2
to
43d035a
Compare
Pool offers a similar behavior to simpy.Container with some additional events: * Pool.when_any() when the pool is non-empty * Pool.when_new() when items are inserted in pool * Pool.when_full() when the pool is full Pool is to simpy.Container what Queue is to simpy.Store
* Fix a bug in when testing when_new event * Add case when a consumer needs to wait a producer
desmod/pool.py
Outdated
| "Amount {} greater than pool's {} capacity {}".format( | ||
| get_ev.amount, str(self.name), self.capacity)) | ||
| if get_ev.amount <= self.level: | ||
| self._getters.remove(get_ev) |
There was a problem hiding this comment.
The control flow here is suspicious since we're removing from the container we're iterating (_getters). I think we need a while loop instead of a for loop. It might be helpful to reference simpy's BaseResource._trigger_put() which uses a while loop for, I believe, this very reason.
desmod/pool.py
Outdated
| """ | ||
|
|
||
| def __init__(self, env, capacity=float('inf'), hard_cap=False, | ||
| init_level=0, name=None): |
There was a problem hiding this comment.
I suggest renaming init_level to init for compatibility with simpy.Container.
For the same reason, I also suggest the parameter order be (env, capacity, init, hard_cap, name) so that Container's specifying init positionally can be trivially replaced with Pool.
* Fix unsafe loop in Pool._trigger_get * Add check that Pool.put and Pool.get take only positive values * Add tests to validate correct handling of Pool.put(0) and Pool.get(0)
|
Thanks @vagvaz! |
Pool offers a similar behavior to simpy.Container with some
additional events:
Pool is to simpy.Container what Queue is to simpy.Store