Skip to content

Fix order of operations in codel control law#2479

Merged
robgjansen merged 1 commit intoshadow:mainfrom
robgjansen:codel_law_ops
Oct 20, 2022
Merged

Fix order of operations in codel control law#2479
robgjansen merged 1 commit intoshadow:mainfrom
robgjansen:codel_law_ops

Conversation

@robgjansen
Copy link
Copy Markdown
Member

@robgjansen robgjansen commented Oct 19, 2022

Fixes #2478

@robgjansen robgjansen added Type: Bug Error or flaw producing unexpected results Component: Main Composing the core Shadow executable labels Oct 19, 2022
@robgjansen robgjansen self-assigned this Oct 19, 2022
@codecov
Copy link
Copy Markdown

codecov bot commented Oct 19, 2022 •
edited
Loading

Codecov Report

Base: 43.16% // Head: 43.73% // Increases project coverage by +0.56% 🎉

Coverage data is based on head (df2288b) compared to base (d3c7445).
Patch has no changes to coverable lines.

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

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2479      +/-   ##
==========================================
+ Coverage   43.16%   43.73%   +0.56%     
==========================================
  Files         186      186              
  Lines       27307    27307              
  Branches     5417     5417              
==========================================
+ Hits        11788    11943     +155     
+ Misses      12865    12678     -187     
- Partials     2654     2686      +32
Flag Coverage Δ
tests 43.73% <ø> (+0.56%) ⬆️

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

Impacted Files Coverage Δ
src/main/core/scheduler/thread_per_host.rs 93.42% <0.00%> (-1.32%) ⬇️
src/lib/shadow-shim-helper-rs/src/scmutex.rs 86.74% <0.00%> (-0.61%) ⬇️
src/main/host/descriptor/socket/unix.rs 68.13% <0.00%> (-0.23%) ⬇️
src/main/host/descriptor/mod.rs 66.44% <0.00%> (+0.43%) ⬆️
src/lib/shadow-shim-helper-rs/src/signals.rs 70.70% <0.00%> (+0.63%) ⬆️
src/main/core/sim_config.rs 54.54% <0.00%> (+0.86%) ⬆️
src/main/network/network_graph.rs 39.27% <0.00%> (+1.12%) ⬆️
src/main/host/memory_manager/memory_copier.rs 74.71% <0.00%> (+1.14%) ⬆️
src/main/host/descriptor/descriptor_table.rs 81.48% <0.00%> (+1.23%) ⬆️
src/main/host/memory_manager/mod.rs 79.16% <0.00%> (+1.47%) ⬆️
... and 11 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.

@robgjansen robgjansen mentioned this pull request Oct 19, 2022
Merged
stevenengler
stevenengler approved these changes Oct 19, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@stevenengler stevenengler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch! I see you're running a benchmark, so it will be nice to know what the impact is on the tor network performance.

@robgjansen
Copy link
Copy Markdown
Member Author

robgjansen commented Oct 20, 2022

Benchmark results show that performance is non-deterministic but not changed too much:
https://github.com/shadow/benchmark-results/tree/master/tor/2022-10-19-T15-26-44

@robgjansen robgjansen enabled auto-merge October 20, 2022 00:07
@robgjansen robgjansen merged commit 9b45850 into shadow:main Oct 20, 2022
@robgjansen robgjansen mentioned this pull request Oct 26, 2022
Merged
@robgjansen
Copy link
Copy Markdown
Member Author

robgjansen commented Oct 26, 2022

Manually ran a larger 20% Tor network using tornettools, here are the results: tornet-codel-bugfix.plot.pages.pdf

Based on the results, the CoDel bug didn't appear to affect Tor network performance enough to lead us to believe that previous Tor simulations are invalid.

@robgjansen robgjansen deleted the codel_law_ops branch January 2, 2024 01:24
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

@robgjansen robgjansen

Labels

Component: Main Composing the core Shadow executable Type: Bug Error or flaw producing unexpected results

Projects

None yet

Milestone

Improve modularity of router object

Development

Successfully merging this pull request may close these issues.

Incorrect order of operations in CoDel control law implementation 🤦

2 participants

@robgjansen @stevenengler