Supporting unordered detach operation #823
Conversation
ipdemes
added
the
category:bug-fix
ipdemes
changed the title
legate/core/store.py
Outdated
| if self.detach_future and not self.detach_future.is_ready(): | ||
| self.detach_future.wait() |
There was a problem hiding this comment.
I don't think this is the right place to do this check. it should be done when the region field is recycled. and the recycling logic should be tweaked such that it favors free region fields with no pending detach over those with one.
There was a problem hiding this comment.
@magnatelee : I believe I have added requested logic. Do you think I should still keep the check here? Can there be the case when RegionField is re-attached without going through the FieldManager ?
There was a problem hiding this comment.
Do you think I should still keep the check here?
The check should be changed to an assertion or an exception, as the store at this line shouldn't have any outstanding detach op.
Can there be the case when RegionField is re-attached without going through the FieldManager ?
Re-attaching is illegal. Look at the check at line 160.
|
/ok to test |
legate/core/runtime.py
Outdated
| else: | ||
| free_fields.append(field_info) | ||
| count -= 1 | ||
| # FIXME: should I return None here instead waiting for future? |
There was a problem hiding this comment.
@magnatelee : what would you suggest doing in the case when all fields in the free_fields container have not been deallocated yet: should we call wait on the detach_future or return None?
There was a problem hiding this comment.
Let's wait on the future for now. Though any stores with attachments are expected to be small (otherwise, attached allocations couldn't even have been created in a single memory), we don't want to spew stores in the pathological case where the client code keeps creating a store and attaching an allocation to it.
| # guaranteed to be ordered across all the shards even with | ||
| # control replication | ||
| self.free_fields: Deque[tuple[Region, int]] = deque() | ||
| self.free_fields: Deque[ |
There was a problem hiding this comment.
I feel _try_reuse_field would become much simpler if we had two deques, one for those free of pending detach ops and one for those with them.
|
@ipdemes I didn't do a thorough review on this, as we know that this code won't be maintained in the future. Let's just merge it for now and move on. |
Co-authored-by: Manolis Papadakis <manopapad@gmail.com>
Co-authored-by: Manolis Papadakis <manopapad@gmail.com>
Co-authored-by: Manolis Papadakis <manopapad@gmail.com>
manopapad
left a comment
manopapad
left a comment
There was a problem hiding this comment.
One minor suggestion, otherwise looks good
4 participants
No description provided.