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

Refill token buckets less often on slow connections #2707

Merged
merged 1 commit into from
Jan 31, 2023

Conversation

robgjansen
Copy link
Member

The token bucket now returns the time that the next packet conforms, ie how long until we will have enough tokens to forward that packet. Returning this instead of how long until the next refill reduces the overall number of forwarding events we need to schedule, and is especially helpful on slow connections.

@robgjansen robgjansen added Type: Enhancement New functionality or improved design Component: Main Composing the core Shadow executable labels Jan 29, 2023
@robgjansen robgjansen self-assigned this Jan 29, 2023
@codecov
Copy link

codecov bot commented Jan 29, 2023

Codecov Report

Base: 67.21% // Head: 68.14% // Increases project coverage by +0.92% 🎉

Coverage data is based on head (d5e9187) compared to base (88e32b7).
Patch coverage: 64.51% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2707      +/-   ##
==========================================
+ Coverage   67.21%   68.14%   +0.92%     
==========================================
  Files         203      203              
  Lines       30484    30515      +31     
  Branches     5961     5971      +10     
==========================================
+ Hits        20491    20795     +304     
+ Misses       5329     5004     -325     
- Partials     4664     4716      +52     
Flag Coverage Δ
tests 68.14% <64.51%> (+0.92%) ⬆️

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

Impacted Files Coverage Δ
src/main/network/relay/mod.rs 83.51% <50.00%> (+12.72%) ⬆️
src/main/network/relay/token_bucket.rs 69.53% <65.51%> (-0.17%) ⬇️
src/lib/shmem/src/scmutex.rs 87.05% <0.00%> (-2.36%) ⬇️
src/main/core/scheduler/pools/bounded.rs 74.50% <0.00%> (-1.97%) ⬇️
src/lib/tsc/tsc_test.c 12.67% <0.00%> (ø)
src/main/utility/byte_queue.rs 56.01% <0.00%> (ø)
src/main/utility/shm_cleanup.rs 61.36% <0.00%> (ø)
src/main/host/syscall/handler/socket.rs 66.66% <0.00%> (+0.22%) ⬆️
src/lib/shadow-shim-helper-rs/src/signals.rs 72.28% <0.00%> (+0.60%) ⬆️
src/main/host/descriptor/mod.rs 68.29% <0.00%> (+0.81%) ⬆️
... and 22 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
Copy link
Member Author

Tor benchmark results seem reasonable:
https://github.com/shadow/benchmark-results/tree/master/tor/2023-01-29-T15-04-36

src/main/network/relay/mod.rs Outdated Show resolved Hide resolved
src/main/network/relay/token_bucket.rs Outdated Show resolved Hide resolved
src/main/network/relay/token_bucket.rs Outdated Show resolved Hide resolved
src/main/network/relay/token_bucket.rs Outdated Show resolved Hide resolved
@stevenengler
Copy link
Contributor

stevenengler commented Jan 31, 2023

The simulation did seem to get a bit slower in the tor benchmark, but I would have expected the opposite. Maybe worth waiting for tonight's nightly to run, rebasing this PR, and then running the simulation again to check if it was just within error since the weekly has large error bars. But I'm fine either way since I don't see anything wrong with this PR.

1675127343_grim

@robgjansen
Copy link
Member Author

robgjansen commented Jan 31, 2023

Rebased and ran another Tor benchmark as suggested, and the performance is in line with the nightly from last night:
(https://github.com/shadow/benchmark-results/tree/master/tor/2023-01-31-T14-09-13)

run_time

(We currently think the regression on the nightly was from #2632.)

The token bucket now returns the time that the next packet conforms,
ie how long until we will have enough tokens to forward that packet.
Returning this instead of how long until the next refill reduces the
overall number of forwarding events we need to schedule, and is
especially helpful on slow connections.
@robgjansen robgjansen enabled auto-merge (squash) January 31, 2023 22:37
@robgjansen robgjansen merged commit b093f96 into shadow:main Jan 31, 2023
@robgjansen robgjansen deleted the smarter-token-bucket branch January 31, 2023 23:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Main Composing the core Shadow executable Type: Enhancement New functionality or improved design
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants