Refactor EvalState to not use Arcs, label Store's significant drops#59
Refactor EvalState to not use Arcs, label Store's significant drops#59Naxdy wants to merge 3 commits into
EvalState to not use Arcs, label Store's significant drops#59Conversation
roberth
left a comment
roberth
left a comment
There was a problem hiding this comment.
- Currently degrades memory safety.
- Have questions, see comments
| /// An abstraction over the underlying eval state. | ||
| /// | ||
| /// When an `EvalState` is constructed, it will allocate a number of threads to be used for | ||
| /// evaluating expressions. These threads will remain allocated until the `EvalState` is dropped. |
There was a problem hiding this comment.
This could be true of C++ EvalState, but I'm not sure if that's relevant to document here. It also does many other things.
It begs the question though: how can we have multiple &mut EvalState?
We seem to have one for each PrimopState!
That would be ok if EvalState had been merely a smart pointer, but then we wouldn't need mut, I assume. However the comment contradicts that.
| // We'll be leaking this Box. | ||
| // TODO: Use the GC with finalizer, if possible. | ||
| let user_data = ManuallyDrop::new(Box::new(PrimOpContext { | ||
| let user_data = Box::leak(Box::new(PrimOpContext { |
There was a problem hiding this comment.
now protected against by the borrow checker.
Unfortunately we've created a false sense of security here. This Box::leak lets the &mut EvalState stick around user_data, effectively untracked by the borrow checker, and by removing the weak reference functionality, you've removed the runtime check, creating UB instead of a panic if it were to go wrong.
2 participants
This PR refactors the
EvalStateto no longer use an internalArcand remove itsCloneimplementation. The reason for this is that anEvalStateallocates several resources over its lifetime that are held until it isDropped.Downstream crates can still wrap it in
Rcif they needClone, orArc<Mutex<>>if they also needSend + Sync. This also has the advantage of maintaining a singleContextacross clones.This also inadvertently fixes a potential issue with
PrimOpwhere we used to accessEvalStatevia a mutable reference constructed from a raw pointer (while potentially still holding references elsewhere), which is now protected against by the borrow checker.As for the
Store, the innerArcis maintained (for now), due to the need to work around NixOS/nix#11979 - but theDropsignificance is now clearly labeled & documented.Upstream of DeterminateSystems#15