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

Don't refcount Process #2601

Merged
merged 12 commits into from
Dec 12, 2022
Merged

Don't refcount Process #2601

merged 12 commits into from
Dec 12, 2022

Conversation

sporksmith
Copy link
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

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
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.

src/main/host/descriptor/tcp.c Outdated Show resolved Hide resolved
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.
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.

None yet

2 participants