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

Correctly implement the cpuRateLimit feature #2189

Merged
merged 6 commits into from
Mar 31, 2022

Conversation

tomaka
Copy link
Contributor

@tomaka tomaka commented Mar 31, 2022

Correctly implements #2151

The code currently does u32::max_value().checked_sub(rate_limit). This is wrong, it should be checked_div.
I remember that when I was testing #2151 I was doing u32::max_value() / rate_limit, and then at the last second before opening the PR I replaced this with the checked_ function in order to handle the situation where rate_limit is 0.

In addition to fixing this mistake, I've also changed the code to use floating points instead of integers.
Right now the precision is actually very low: because we round the division to the nearest integer, the possible values for the CPU bound can only be 100%, 50%, 25%, 12.5%, etc. Using floating points fixes this.

Copy link
Contributor

@mergify mergify bot left a comment

Choose a reason for hiding this comment

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

Automatically approving tomaka's pull requests. This auto-approval will be removed once more maintainers are active.

@@ -38,6 +38,7 @@ const client = smoldot.start({
forbidWs: false,
forbidNonLocalWs: false,
forbidWss: false,
cpuRateLimit: 0.5,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: I've intentionally left this in the demo so that it mimics what substrate-connect will now do (paritytech/substrate-connect#900)

Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

👍

poll_duration.as_secs_f64() * *this.max_divided_by_rate_limit_minus_one;
debug_assert!(after_poll_sleep >= 0.0 && !after_poll_sleep.is_nan());
let max_duration_float: f64 = Duration::MAX.as_secs_f64(); // TODO: turn this into a `const` once `as_secs_f64` is `const`
if !(after_poll_sleep < max_duration_float) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if !(after_poll_sleep < max_duration_float) {
if after_poll_sleep >= max_duration_float {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this is intentional because it's not exactly the same!
If after_poll_sleep is NaN, then !(after_poll_sleep < max_duration_float) will be true but after_poll_sleep >= max_duration_float will be false.
It's not super important because it's never supposed to be NaN, but IMO it doesn't hurt to have it this way.

let max_divided_by_rate_limit_minus_one =
(f64::from(u32::max_value()) / f64::from(rate_limit)) - 1.0;
// Note that it is legal for `max_divided_by_rate_limit_minus_one` to be +infinite
debug_assert!(
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a guidance on when to use debug_asserts? don't you think they tend to clutter the code?

Copy link
Contributor Author

@tomaka tomaka Mar 31, 2022

Choose a reason for hiding this comment

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

I don't think I've written this down, but I use them for internal-bug-detecting purposes.
In other words, what you put in the debug_assert! is something that, if false, indicates that there's a bug in the internal logic of the current module.
However if it's an incorrect API usage, then it should be assert! instead.

The reason is that incorrect API usages tend to happen not because of silly programming mistakes but because it's sometimes hard to understand or keep in mind how the API that you're using is supposed to be used, and thus are more likely to happen and to pass code reviews.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 31, 2022

twiggy diff report

Difference in .wasm size before and after this pull request.


 Delta Bytes │ Item
─────────────┼──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
        -393 ┊ <smoldot_light_wasm::cpu_rate_limiter::CpuRateLimiter<T> as core::future::future::Future>::poll::hd1b3692f1a559a7f
        +335 ┊ core::time::Duration::from_secs_f64::h6350bd82a690aa63
        +274 ┊ <smoldot_light_wasm::cpu_rate_limiter::CpuRateLimiter<T> as core::future::future::Future>::poll::h637b1d9f092ce30e
        -269 ┊ <smoldot_light_wasm::platform::Platform as smoldot_light_base::Platform>::now_from_unix_epoch::h2cb6578f5b4acc50
         +83 ┊ core::ptr::drop_in_place<smoldot_light_wasm::cpu_rate_limiter::CpuRateLimiter<core::future::from_generator::GenFuture<smoldot_light_wasm::init::init<alloc::vec::Vec<futures_util::abortable::AbortHandle>,smoldot_light_wasm::platform::Platform>::{{closure}}>>>::hedc2b33abe2a5ffb
         -75 ┊ core::ptr::drop_in_place<smoldot_light_wasm::cpu_rate_limiter::CpuRateLimiter<core::future::from_generator::GenFuture<smoldot_light_wasm::init::init<alloc::vec::Vec<futures_util::abortable::AbortHandle>,smoldot_light_wasm::platform::Platform>::{{closure}}>>>::hd5f4a881f3302fb8
         +18 ┊ init
         +34 ┊ Σ [7 Total Rows]

@tomaka tomaka added the automerge Automatically merge pull request as soon as possible label Mar 31, 2022
@mergify mergify bot merged commit 591b9d5 into paritytech:main Mar 31, 2022
@tomaka tomaka deleted the cpu-rate-limit-correct branch March 31, 2022 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Automatically merge pull request as soon as possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants