Use a no-op order manager for in-order queues#2299
Use a no-op order manager for in-order queues#2299ndgrigorian wants to merge 5 commits intomasterfrom
Conversation
3784662 to
849b0af
Compare
|
View rendered docs @ https://intelpython.github.io/dpctl/pulls/2299/index.html |
849b0af to
115b184
Compare
|
@antonwolfy @vlad-perevezentsev Please experiment with it as well |
| def __getitem__(self, q: SyclQueue) -> _SequentialOrderManager: | ||
| def __getitem__( | ||
| self, q: SyclQueue | ||
| ) -> Union[_SequentialOrderManager, _NoOpOrderManager]: |
There was a problem hiding this comment.
Would it make sense to implement a protocol instead of union to have a pure type-checking construct?
from typing import Protocol
class OrderManagerProtocol(Protocol):
"""Protocol defining order manager interface"""
def add_event_pair(self, host_task_ev, comp_ev) -> None: ...
@property
def num_host_task_events(self) -> int: ...
# ... etc
def __getitem__(self, q: SyclQueue) -> OrderManagerProtocol:
# type checkers understand this is an interfaceThere was a problem hiding this comment.
we could, though also, we could just use | instead of the Union since we restrict to >=3.10 anyway.
I'll try the protocol approach and see if it saves some redundancy
| raise TypeError(f"Expected `dpctl.SyclQueue`, got {type(q)}") | ||
| if q.is_in_order: | ||
| # we don't need to cache the NoOpOrderManager since it's stateless | ||
| return _NoOpOrderManager(q) |
There was a problem hiding this comment.
Is it expected there will be no wait() called for that queue on clear method below?
There was a problem hiding this comment.
I wonder about finalization of tasks from in-order queue at exit
There was a problem hiding this comment.
Is it expected there will be no
wait()called for that queue onclearmethod below?
I think it makes sense in the sense that, no events were present in it to begin with. The original SequentialOrderManager calls the wait on its own events, but this order manager, owns no events. But I see your point about the finalization though, it's fair: unlike the normal order manager, this one wouldn't clear.
We will need to track some queues, I think.
As pointed out in #2293, the
SequentialOrderManagerconstruct may not bring any benefit with in-order queues, and, with an in-order queue, an extension/function implementation has to insert events into theSequentialOrderManagerif using library calls which also useSequentialOrderManagerunnecessarily.To remedy this, this PR proposes adding a
_NoOpOrderManagerimplementation class thatSequentialOrderManagercan use when passed an in-order queue.The returned object isn't cached, as it's mostly a no-op. Notably, the
_NoOpOrderManagerstores the in-order queue that it was given, so thatwait()can callq.wait()so that the method is still meaningful.