enhance: async cgo utility #33133
enhance: async cgo utility #33133
Conversation
sre-ci-robot
added
size/XXL
mergify
bot
added
dco-passed
|
@chyezh ut workflow job failed, comment |
|
@chyezh E2e jenkins job failed, comment |
|
@chyezh ut workflow job failed, comment |
chyezh
force-pushed
the
feat_async_cgo
branch
2 times, most recently
from
4506cda
to
e22eca1
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #33133 +/- ##
==========================================
+ Coverage 81.23% 81.38% +0.15%
==========================================
Files 1029 1038 +9
Lines 132215 133169 +954
==========================================
+ Hits 107399 108377 +978
+ Misses 20766 20725 -41
- Partials 4050 4067 +17
|
chyezh
force-pushed
the
feat_async_cgo
branch
2 times, most recently
from
6d7e8cb
to
de513be
Compare
|
@chyezh ut workflow job failed, comment |
mergify
bot
added
needs-dco
|
@chyezh Thanks for your contribution. Please submit with DCO, see the contributing guide https://github.com/milvus-io/milvus/blob/master/CONTRIBUTING.md#developer-certificate-of-origin-dco. |
mergify
bot
added
dco-passed
|
@chyezh ut workflow job failed, comment |
|
@chyezh E2e jenkins job failed, comment |
|
@chyezh ut workflow job failed, comment |
|
@chyezh ut workflow job failed, comment |
chyezh
force-pushed
the
feat_async_cgo
branch
2 times, most recently
from
de94aa1
to
a0f98ef
Compare
|
rerun ut |
1 similar comment
|
rerun ut |
| num_threads, | ||
| std::make_unique<folly::PriorityLifoSemMPMCQueue< | ||
| folly::CPUThreadPoolExecutor::CPUTask, | ||
| folly::QueueBehaviorIfFull::BLOCK>>(num_priority, |
There was a problem hiding this comment.
this might be the reason thread will increase.
might be reasonable to throw error under queue full?
There was a problem hiding this comment.
- number of threads is fixed, not increase in pool.
- current behaviour is blocked if queue is full, these will cause a long cgo calling.
- throwing handling will increase the code complexity, it should collaborate with go-side to avoid the queue is full.
internal/util/cgo/futures.go
Outdated
| state: newFutureState(), | ||
| } | ||
|
|
||
| runtime.SetFinalizer(future, func(future *futureImpl) { |
There was a problem hiding this comment.
Do we really need finalizer?
could we can handle future destroy in the select loop?
There was a problem hiding this comment.
cancel and destroy might be happen in the same call?
There was a problem hiding this comment.
- destroy is just a light-weight destruction of C-side, I think it's ok to set it as a finializer to avoid resource leak.
- cancel just notify the underlying cancellation token, destroy can be only called if the underlying task is done. So cannot merge it together.
| } | ||
|
|
||
| func (f *futureImpl) BlockAndLeakyGet() (unsafe.Pointer, error) { | ||
| f.blockUntilReady() |
There was a problem hiding this comment.
same here, blockUntil ready and future leak get and get and be one cgo?
There was a problem hiding this comment.
- blockUntilready can be called multiple times, but leakyget can be only call once. It's better to implement two method to perform a future-like api.
- we cannot merge it together, there are always two cgo call will be applied
future_go_register_ready_callback, ``future_leak_and_get` to avoid long cgo call.
|
@chyezh E2e jenkins job failed, comment |
- implement future-based cgo utility. Signed-off-by: chyezh <[email protected]>
Signed-off-by: chyezh <[email protected]>
Signed-off-by: chyezh <[email protected]>
Signed-off-by: chyezh <[email protected]>
Signed-off-by: chyezh <[email protected]>
Signed-off-by: chyezh <[email protected]>
Signed-off-by: chyezh <[email protected]>
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chyezh, xiaofan-luan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
issue: #30926, #33132