Use jl_adopt_thread instead of AsyncCondition for interop
#39
Conversation
…object_store upgrade
|
The CI is red -- we maybe need to bump |
Yes, I am just waiting on conclusion of the review here to release |
| config = into_config(conf) | ||
| while true | ||
| result = GC.@preserve buffer config response cond begin | ||
| preserve_task(ct) |
There was a problem hiding this comment.
I don't understand this. Why would we need to GC-preserve our own task, if we're also blocking, waiting for the event to finish? Of course the task will be kept alive since the task itself is waiting on the event. ...
... oh, is the issue that the Event itself is only rooted from our own stack, and the task is only rooted by the Event's wait-queue, so it's a cycle and that cycle can get GC'd!? 😅 😅 😅 Wow, that's wild.
I think that this deserves some comments explaining this, and explaining why the preserve is needed.
There was a problem hiding this comment.
Also, why couldn't this use Base.preserve_handle(ct)? Is it to reduce contention?
Please clarify this in comments. 🙏
| result = GC.@preserve buffer config response event try | ||
| result = @ccall rust_lib.put( | ||
| path::Cstring, | ||
| buffer::Ref{Cuchar}, | ||
| size::Culonglong, | ||
| config::Ref{Config}, | ||
| response::Ref{Response}, | ||
| cond_handle::Ptr{Cvoid} | ||
| handle::Ptr{Cvoid} | ||
| )::Cint | ||
|
|
||
| wait_or_cancel(cond, response) | ||
| wait_or_cancel(event, response) | ||
|
|
||
| result | ||
| finally | ||
| unpreserve_task(ct) | ||
| end | ||
|
|
||
| if result == 2 | ||
| # backoff | ||
| sleep(1.0) | ||
| sleep(0.01) | ||
| continue | ||
| end |
There was a problem hiding this comment.
Question: Why do we check result == 2, after waiting? The wait_or_cancel cannot affect the value of result, so if result == 2 means we need to retry, why even wait?
I think this could use a comment, and/or a global constant that gives a meaningful name to whatever 2 is.
| # and should thus not be garbage collected. | ||
| # This copies the behavior of Base.preserve_handle. | ||
| const tasks_in_flight = IdDict{Task, Int64}() | ||
| const preserve_task_lock = Threads.SpinLock() |
There was a problem hiding this comment.
Last comment: if we're still seeing high contention here, it could make sense to shard this into multiple dicts and locks, keyed by a hash of the task's objectid? That would be a simple way to significantly reduce contention on the spinlock. Ideally, we'd be able to register a new IO without any spinning at all. I think even just 10 or 100 shards should be enough to completely eliminate that, yeah?
This PR introduces a number of improvements and fixes to RustyObjectStore: