Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Get rid of IDs for synchronization primitives, make pthread shims not leaky #3967

Open
RalfJung opened this issue Oct 12, 2024 · 6 comments
Open
Labels
A-concurrency Area: affects our concurrency (multi-thread) support A-unix Area: affects our shared Unix target support C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement E-good-first-issue A good way to start contributing, mentoring is available

Comments

@RalfJung
Copy link
Member

RalfJung commented Oct 12, 2024

We currently store the state of our synchronization primitives with a layer of indirection: there are lists in SynchronizationObjects and then we use IDs to index into these lists. These lists keep growing while the program runs and never get cleaned up, which isn't very clean and also makes @saethlin unhappy. ;)

As of #3966, we no longer store these IDs in machine memory, so there isn't actually a good reason to still use IDs. Instead, the PthreadMutex used by pthread_mutex_t can just directly store a sync::Mutex. That way, when the allocation that contains the mutex is freed, we also free all the extra data associated with the mutex, thus avoiding memory leaks.

The main trouble here is functions like mutex_unlock(&mut self, id: MutexId): these would want to take a reference to the Mutex instead, but since they also take a mutable reference to the entire machine state, that can't work! We could carefully arrange things to avoid overlapping references here... but probably the easier approach is to use reference counting. So we'd make PthreadMutex store a MutexRef (which is a newtype for Rc<RefCell<Mutex>>) and the function signatures would look like

fn mutex_unlock(&mut self, id: &MutexRef) -> ...

#3971 does this for Futex so that can serve as a model.

Currently we are also using these IDs in BlockReason, but that's just a sanity check, so it's okay to remove them there.

Also as part of this, we could finally fix the leaks in pthread_mutex_destroy and friends.

@RalfJung RalfJung added C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement E-good-first-issue A good way to start contributing, mentoring is available A-concurrency Area: affects our concurrency (multi-thread) support A-unix Area: affects our shared Unix target support labels Oct 12, 2024
@tiif
Copy link
Contributor

tiif commented Oct 12, 2024

I still have a few things to clear in my todo list, but I'd like to work on a different part of the codebase, so,

@rustbot claim

@RalfJung
Copy link
Member Author

Note that this is blocked on #3966

@RalfJung RalfJung changed the title Get rid of IDs for synchronization primitives Get rid of IDs for synchronization primitives, make pthread shims not leaky Oct 13, 2024
@tiif
Copy link
Contributor

tiif commented Oct 26, 2024

@RalfJung Is this also blocked on #3971? If not I could start working on this soon :)

@RalfJung
Copy link
Member Author

Is this also blocked on #3971?

No. That PR can just be helpful as a guide for what this could look like -- there we have Futex and FutexRef, so here we'd have e.g. Mutex and MutexRef.

@tiif
Copy link
Contributor

tiif commented Nov 25, 2024

@rustbot release-assignment

I have quite a few things on my queue right now. Current SynchronizationObjects is left with three lists, which I expect all of them could be removed.

pub struct SynchronizationObjects {
    rwlocks: IndexVec<RwLockId, RwLock>,
    condvars: IndexVec<CondvarId, Condvar>,
    pub(super) init_onces: IndexVec<InitOnceId, InitOnce>,
}

@RalfJung
Copy link
Member Author

#4002 can serve as a good template for what removing one of these lists looks like.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-concurrency Area: affects our concurrency (multi-thread) support A-unix Area: affects our shared Unix target support C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement E-good-first-issue A good way to start contributing, mentoring is available
Projects
None yet
Development

No branches or pull requests

2 participants