Skip to content

Plan for interior mutability in Thread / ThreadRef#2703

Merged
sporksmith merged 4 commits intoshadow:mainfrom
sporksmith:thread-immut
Jan 27, 2023
Merged

Plan for interior mutability in Thread / ThreadRef#2703
sporksmith merged 4 commits intoshadow:mainfrom
sporksmith:thread-immut

Conversation

@sporksmith
Copy link
Copy Markdown
Contributor

@sporksmith sporksmith commented Jan 27, 2023 •
edited
Loading

At a high level, I think we're going to need interior mutability in Thread/ThreadRef for similar reasons as in Process and Host - it'd be difficult to avoid incompatible borrows of Thread itself without it.

More concretely, I ran into a problem when changing SysCallCondition to store a tid instead of a Thread*: some SysCallCondition methods need to act on the Thread. Since the condition itself is a member of the Thread, it'll be impossible to simultaneously hold a &mut Thread and a reference to the condition. Using a &Thread instead of a &mut Thread works around this.

I went a bit further here than strictly necessary and removed the RootedRefCell wrapper around Process's ThreadRefs altogether. I think as with Process we'll end up making ~all methods take &self instead of &mut self, so will never need to get a &mut reference to the ThreadRef.

We could leave the RootedRefCell in place to make things easier in case we want to get &mut references later, but after working through the analagous problem in Process I think I'm leaning towards following the yagni principle here. Better to make the code simpler now and only support as much functionality as we know we need.

@sporksmith sporksmith self-assigned this Jan 27, 2023
@github-actions github-actions bot added the Component: Main Composing the core Shadow executable label Jan 27, 2023
@sporksmith
Copy link
Copy Markdown
Contributor Author

sporksmith commented Jan 27, 2023

I think we could analogously remove the RootedRefCell around the Processs in the Host's process list, but I think I'll work through migrating Thread with this approach before going back and messing with Process.

With the RootedRefCells in both places gone, we could think about removing the RootedRc wrappers as well, but that'd make it so that we couldn't mutate the process and thread lists while holding a reference to a process or thread. There are only a couple places where we actually need to mutate those lists though, so we could probably work around that by scheduling an event to do the mutation, or maybe using a CallbackQueue.

@sporksmith
Copy link
Copy Markdown
Contributor Author

sporksmith commented Jan 27, 2023

Oops, fixing clippy...

@sporksmith sporksmith removed the request for review from stevenengler January 27, 2023 16:57
@sporksmith
Thread list: remove RootedRefCell wrapper around each Thread
71acc3a 
I don't think we're going to have `&mut` methods on Thread. Therefore we
don't need to get `&mut` references to threads. Therefore we don't need
this wrapper.
@sporksmith
MemoryManager: don't require &mut ThreadRef
b99e497
@sporksmith
ThreadContext: use &ThreadRef instead of &mut ThreadRef
322cec4
@codecov
Copy link
Copy Markdown

codecov bot commented Jan 27, 2023 •
edited
Loading

Codecov Report

Base: 68.09% // Head: 68.17% // Increases project coverage by +0.08% 🎉

Coverage data is based on head (322cec4) compared to base (b349261).
Patch coverage: 85.71% of modified lines in pull request are covered.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2703      +/-   ##
==========================================
+ Coverage   68.09%   68.17%   +0.08%     
==========================================
  Files         202      202              
  Lines       30414    30398      -16     
  Branches     5949     5948       -1     
==========================================
+ Hits        20710    20724      +14     
+ Misses       5000     4969      -31     
- Partials     4704     4705       +1
Flag Coverage Δ
tests 68.17% <85.71%> (+0.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/main/host/context.rs 61.29% <66.66%> (ø)
src/main/host/thread.rs 81.11% <80.00%> (ø)
src/main/host/process.rs 78.76% <86.36%> (+0.06%) ⬆️
src/main/host/host.rs 80.54% <100.00%> (ø)
src/main/host/memory_manager/memory_mapper.rs 71.40% <100.00%> (ø)
src/main/host/memory_manager/mod.rs 67.74% <100.00%> (ø)
src/main/core/logger/shadow_logger.rs 69.61% <0.00%> (-2.77%) ⬇️
src/main/host/syscall/handler/socket.rs 66.44% <0.00%> (-1.55%) ⬇️
src/main/host/descriptor/pipe.rs 83.75% <0.00%> (-0.63%) ⬇️
...c/main/utility/synchronization/count_down_latch.rs 85.10% <0.00%> (-0.54%) ⬇️
... and 12 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

stevenengler
stevenengler approved these changes Jan 27, 2023
View reviewed changes
@sporksmith sporksmith merged commit 0a2b8ff into shadow:main Jan 27, 2023
@sporksmith sporksmith deleted the thread-immut branch January 27, 2023 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stevenengler stevenengler stevenengler approved these changes

Assignees

@sporksmith sporksmith

Labels

Component: Main Composing the core Shadow executable

Projects

None yet

Milestone

Convert existing modules to Rust

Development

Successfully merging this pull request may close these issues.

2 participants

@sporksmith @stevenengler