Skip to content

Don't refcount Process#2601

Merged
sporksmith merged 12 commits intoshadow:mainfrom
sporksmith:owning-process
Dec 12, 2022
Merged

Don't refcount Process#2601
sporksmith merged 12 commits intoshadow:mainfrom
sporksmith:owning-process

Conversation

@sporksmith
Copy link
Copy Markdown
Contributor

This simplifies the migration of Process to Rust.

I had initially planned to used RootedRc, and to store RootedRcWeak in tasks and to avoid circular references, but I realized the only strong reference would currently be in the Host. It'll be simpler to avoid exposing the RootedRc and RootedRcWeak wrappers around Process to the C code, and this allows us to do it.

This adds some BTreeTable lookups to fetch Process*'s from the Host's process table, but these should be quite fast, especially since most simulations use fairly few processes per host. I'll do a benchmark run to verify, though.

@sporksmith sporksmith self-assigned this Dec 10, 2022
@github-actions github-actions bot added Component: Build Build/install tools and dependencies Component: Main Composing the core Shadow executable Component: Testing Unit and integration tests and frameworks labels Dec 10, 2022
@codecov
Copy link
Copy Markdown

codecov bot commented Dec 10, 2022

Codecov Report

Base: 67.00% // Head: 68.11% // Increases project coverage by +1.11% 🎉

Coverage data is based on head (1fba9d1) compared to base (140a5d7).
Patch coverage: 84.84% of modified lines in pull request are covered.

❗ Current head 1fba9d1 differs from pull request most recent head 1a131d3. Consider uploading reports for the commit 1a131d3 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2601      +/-   ##
==========================================
+ Coverage   67.00%   68.11%   +1.11%     
==========================================
  Files         197      198       +1     
  Lines       29182    29125      -57     
  Branches     5740     5737       -3     
==========================================
+ Hits        19553    19839     +286     
+ Misses       5085     4698     -387     
- Partials     4544     4588      +44     
Flag Coverage Δ
tests 68.11% <84.84%> (+1.11%) ⬆️

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

Impacted Files Coverage Δ
src/test/itimer/test_itimer.rs 63.09% <ø> (-1.27%) ⬇️
src/test/test_utils.rs 68.83% <75.00%> (+0.92%) ⬆️
src/main/host/host.rs 81.57% <100.00%> (+0.62%) ⬆️
src/main/host/process.rs 78.12% <100.00%> (+12.50%) ⬆️
src/main/utility/mod.rs 52.98% <100.00%> (ø)
...rc/test/itimer/test_itimer_scheduled_after_exit.rs 100.00% <100.00%> (ø)
src/main/host/syscall/handler/random.rs 47.61% <0.00%> (-4.77%) ⬇️
src/main/utility/enum_passthrough.rs 66.66% <0.00%> (-2.57%) ⬇️
src/main/core/scheduler/pools/bounded.rs 74.50% <0.00%> (-1.97%) ⬇️
src/main/utility/legacy_callback_queue.rs 78.84% <0.00%> (-0.79%) ⬇️
... and 27 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.

@sporksmith
Copy link
Copy Markdown
Contributor Author

sporksmith commented Dec 12, 2022

Looks like there may be a very small performance penalty, but it's pretty much in the noise.

tgen: https://github.com/shadow/benchmark-results/blob/master/tgen/2022-12-12-T12-31-16/plots/shadow.results.pdf
tor: https://github.com/shadow/benchmark-results/blob/master/tor/2022-12-12-T04-32-28/plots/run_time.png

If there is a performance penalty vs using weak references, it should go away as code gets migrated to Rust, where I expect we'll still use them.

This no longer results in a crash in the small_stop_time-shadow test,
since the process start task no longer manipulates the Process reference
count.

Fixes shadow#2514
Also add a test to validate behavior when a process exits while a timer
is still set. This passes, since the itimer uses a wrapper task that
already detects when the timer no longer exists, so doesn't call
_process_itimer_real_expiration.
@sporksmith sporksmith enabled auto-merge December 12, 2022 19:37
@sporksmith sporksmith merged commit 270148c into shadow:main Dec 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Component: Build Build/install tools and dependencies Component: Main Composing the core Shadow executable Component: Testing Unit and integration tests and frameworks

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants