Closed michiel-de-muynck closed 3 years ago
Many thanks for this! It all looks fine, and matches the existing style so nothing to fix up.
One thing that occurred to me, looking at the 100 thread test, is that by quickly creating and dropping an ACellOwner, you're effectively getting a temporary lock on all the ACells. I don't think we want to encourage this pattern. It would be better for the caller to create a single long-lived ACellOwner wrapped in a RwLock or Mutex. Maybe I'll add a note somewhere before releasing the new version.
I have a few other small-ish changes that I want to do before pushing the next version out to crates.io, but I won't leave it too long.
The intent of that test was just to stress-test that everything still works in an extreme scenario, not necessarily as a recommendation that users should use the API like that.
Yes, that's fine. I just want to make sure that I point crate users in the right direction, in case they independently come up with that pattern, not realizing that it is sub-optimal. Anyway, thanks again!
This commit adds two new functions:
TCellOwner::try_new
: checks if aTCellOwner
already exists, and returnsNone
if it does.TCellOwner::wait_for_new
: if aTCellOwner
already exists, it blocks the thread until that existing owner is dropped.Internally, this was implemented using a single static
Condvar
. If await_for_new
call finds that aTCellOwner
with its marker typeQ
already exists, it waits for thisCondvar
. Any time aTCellOwner
is dropped, all threads waiting for thisCondvar
are woken up and check if theTCellOwner
that was dropped was their marker typeQ
.Performance-wise, this means that dropping a
TCellOwner
is now a tiny bit slower (theCondvar::notify_all
call). This should be completely insignificant for any realistic use case. The performance of every other method (includingTCellOwner::new
) is unaffected.TCellOwner::wait_for_new
is just as fast asTCellOwner::new
if there is no need to wait.