Clean idle pooled schedulers periodically #1575
Conversation
ryoqun
force-pushed
the
idle-pooled-scheduler-cleaning
branch
from
9984a5f to
b16e9fe
Compare
ryoqun
changed the title
ryoqun
force-pushed
the
idle-pooled-scheduler-cleaning
branch
2 times, most recently
from
4d47f20 to
1a7e780
Compare
| // Block solScCleaner until we see trashed schedler... | ||
| assert_eq!(pool_raw.trashed_scheduler_inners.lock().unwrap().len(), 1); | ||
| sleepless_testing::at(TestCheckPoint::BeforeTrashedSchedulerCleaned); | ||
|
|
||
| // See the trashed scheduler gone only after solScCleaner did its job... | ||
| sleepless_testing::at(TestCheckPoint::AfterTrashedSchedulerCleaned); | ||
| assert_eq!(pool_raw.trashed_scheduler_inners.lock().unwrap().len(), 0); |
There was a problem hiding this comment.
it turns out this test is racy to begin with... it seems this test isn't racy unless i add new logic to solScCleaner. so, I'm piggybacking this into this pr.
ryoqun
force-pushed
the
idle-pooled-scheduler-cleaning
branch
2 times, most recently
from
63f08ca to
05f5a25
Compare
| @@ -438,6 +438,7 @@ trees = "0.4.2" | |||
| tungstenite = "0.20.1" | |||
| uriparse = "0.6.4" | |||
| url = "2.5.0" | |||
| vec_extract_if_polyfill = "0.1.0" | |||
There was a problem hiding this comment.
according to this page, we're the first consumer of the polyfill crate: https://crates.io/crates/vec_extract_if_polyfill/reverse_dependencies
however, i've confirmed that this crate is almost verbatim copy of what's gated behind the unstable feature flag in our present rust-nightly std source:
$ git diff -w --no-index ~/.cargo/registry/src/index.crates.io-6f17d22bba15001f/vec_extract_if_polyfill-0.1.0/src/lib.rs ~/.rustup/toolchains/nightly-2024-05-02-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/vec/extract_if.rs
...
$ git diff -w --no-index ~/.cargo/registry/src/index.crates.io-6f17d22bba15001f/vec_extract_if_polyfill-0.1.0/src/lib.rs ~/.rustup/toolchains/nightly-2024-05-02-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/vec/mod.rs
...
There was a problem hiding this comment.
I don't understand why we're adding the dependency instead of just using a nightly feature if we really need it? The docs even say this polyfill crate uses it:
This struct is created by Vec::extract_if. See its documentation for more.
There was a problem hiding this comment.
if we only supported building with nightly, it would work.
but, Vec::extract_if() isn't available for stable rust yet...
ryoqun
force-pushed
the
idle-pooled-scheduler-cleaning
branch
from
05f5a25 to
645f8bc
Compare
| @@ -438,6 +438,7 @@ trees = "0.4.2" | |||
| tungstenite = "0.20.1" | |||
| uriparse = "0.6.4" | |||
| url = "2.5.0" | |||
| vec_extract_if_polyfill = "0.1.0" | |||
There was a problem hiding this comment.
I don't understand why we're adding the dependency instead of just using a nightly feature if we really need it? The docs even say this polyfill crate uses it:
This struct is created by Vec::extract_if. See its documentation for more.
unified-scheduler-pool/src/lib.rs
Outdated
| let Ok(mut scheduler_inners) = scheduler_pool.scheduler_inners.lock() else { | ||
| break; | ||
| }; | ||
| // I want to use the fancy ::extract_if() no matter what.... |
There was a problem hiding this comment.
why though? it seems like we can get something similar with retain and collecting a vec inside?
There was a problem hiding this comment.
why though? it seems like we can get something similar with retain and collecting a vec inside?
retain only gives &mut to the predicate closure. So, we can't collect (i.e. take the ownership) in that way. After all, that's the why there's the still unstable ::extract_if().
And ::extract_if() is a perfect match and simplest & fastest way for this use case.
There was a problem hiding this comment.
well, while waiting for lgtm, I managed to write a in-source commentary: 2b90fd1
There was a problem hiding this comment.
yeah agree it's a good match, would just love if the feature were stable. Just concerned with adding dependency on an otherwise unused crate. repo has no readme either.
kind of curious how you found it.
Looking at the repo, it seems fine to me as is - but who knows with upgrades.
There was a problem hiding this comment.
kind of curious how you found it.
well, crates.io's own search functionality didn't work... i just googled...: https://www.google.com/search?q=rust+extract_if lol
Looking at the repo, it seems fine to me as is - but who knows with upgrades.
as for upgrades, that's certainly a possibility. however, our dep is pinned to exact commit hash via Cargo.lock as usual. so, i think it's okay unless we're practicing blind dependabot merges. ;)
also as far as i screened author profile pages (https://github.com/chyyran and https://crates.io/users/chyyran). i think there's only legit activities.
Lastly, I'm planning to advertise this crate at the rust's tracking issue to increase the exposure of this crate across the ecosystem and to move this forward the stabilization.
* Clean idle pooled schedulers periodically
* Fix race in failure_{with,without}_extra_tx
* Improve test
* Use extract_if polyfill
* Write justfication of extract_if with minor opt.
Problem
From #1211:
Summary of Changes
This pr addresses (3).