chore(ctx): replace gstuff constructible with oncelock #2267
Conversation
mm2src/mm2_core/src/mm_ctx.rs
Outdated
| use lazy_static::lazy_static; | ||
| use libp2p::PeerId; | ||
| use mm2_event_stream::{controller::Controller, Event, EventStreamConfiguration}; | ||
| use mm2_metrics::{MetricsArc, MetricsOps}; | ||
| use once_cell::sync::OnceCell; |
There was a problem hiding this comment.
Why not OnceLock from the standard library?
There was a problem hiding this comment.
ah thanks!, I promise, I didn't know this is stable already..also I've had OnceCell on my mind for a long time.
mm2src/mm2_core/src/mm_ctx.rs
Outdated
| // Fast path with try_lock | ||
| // Get a context of type T, optimizing for the common case where the context is already initialized. | ||
| // | ||
| // # Performance Optimization | ||
| // This implementation uses a "fast path" optimization with `try_lock()` because: | ||
| // - In most cases, the context will already be initialized and threads only need to read it | ||
| // - `try_lock()` attempts a non-blocking lock acquisition first, failing fast if locked | ||
| // - Only falls back to blocking lock if the fast path fails | ||
| // | ||
| if let Ok(guard) = ctx.try_lock() { | ||
| if let Some(ctx) = guard.as_ref() { | ||
| if let Ok(typed_ctx) = ctx.clone().downcast::<T>() { | ||
| return Ok(typed_ctx); | ||
| } | ||
| } |
There was a problem hiding this comment.
How is that fast path-ing?! the logic is essentially the same as just locking right away.
lock shouldn't block if the lock isn't acquired by anybody.
There was a problem hiding this comment.
one other thing regarding from_ctx. it's there to support lazy init of some sub-ctxs we have. IDK really how many of these sub-ctx actually benefit/need lazy initialization, if not too many, we should just initialize things early and abandon from_ctx(sub_ctx, init_clousre) in favor of ctx.sub_ctx.get() (assuming we use OnceLock).
There was a problem hiding this comment.
How is that fast path-ing?! the logic is essentially the same as just locking right away.
lock shouldn't block if the lock isn't acquired by anybody.
https://doc.rust-lang.org/std/sync/struct.Mutex.html#method.lock
Acquires a mutex, blocking the current thread until it is able to do so.
This function will block the local thread until it is available to acquire the mutex. Upon returning, the thread is the only thread with the lock held. An RAII guard is returned to allow scoped unlock of the lock. When the guard goes out of scope, the mutex will be unlocked.
https://doc.rust-lang.org/std/sync/struct.Mutex.html#method.try_lock
Attempts to acquire this lock.
If the lock could not be acquired at this time, then Err is returned. Otherwise, an RAII guard is returned. The lock will be unlocked when the guard is dropped.
This function does not block.
There was a problem hiding this comment.
the argument here is how these two calls are placed. placing them consecutively like that doesn't prevent blocking.
try lock + lock right after is just like a single lock.
also,
lock shouldn't block if the lock isn't acquired by anybody.
so both are equivalent
There was a problem hiding this comment.
lock shouldn't block if the lock isn't acquired by anybody.
You're right, however, the combination of try_lock() followed by lock() has a different purpose
Using try_lock() followed by lock() isn’t the same as just using lock(). With try_lock(), you get a quick, non-blocking attempt to grab the lock. This can boost performance when the lock is usually free. If try_lock() fails, only then does the code fall back to lock() and wait for the lock, so in summary,
try_lock() followed by lock() adds a non-blocking check first
There was a problem hiding this comment.
try lock + lock right after is just like a single lock.
I'm not sure how correct this statement is btw. This can only be true if try_lock() fails
There was a problem hiding this comment.
This can only be true if try_lock() fails
exactly!
and if try_lock doesn't fail then lock will not block and this completes the proof of equivalence :)
There was a problem hiding this comment.
I understand this is because of how I've utilized them(placing them together), which make this if try_lock doesn't fail then lock will not block and this completes the proof of equivalence true but with a previous try_lock we can't/shouldn't assume anything about lock acquisition timing in concurrent code.
borngraced
changed the title
borngraced
added
under review
and removed
in progress
#1309
Replaced
Constructiblefrom thegstuffcrate withonce_cell::sync::OnceCellfor managing lazy-initialized values inmm_ctxfor better performance and stronger thread safety.also add a slight improvement to
from_ctx