Improvements #2

NelsonVides · 2023-12-15T21:28:54Z

Reimplement everything using specific bucket sizes and flexible time units.

Let PropER try to shrink, give common-tests assertions instead of failures that will crash when trying to print, have all properties run the same number of tests.

codecov-commenter · 2023-12-15T21:30:02Z

Welcome to Codecov 🎉

Once merged to your default branch, Codecov will compare your coverage reports and display the results in this comment.

Thanks for integrating Codecov - We've got you covered ☂️

chrzaszcz

I took a quick look and spotted issues. Since I think it is quite easy to find problems in this code, I think it needs unit tests. The logic in the update function should be IMO extracted to a pure function (to make it independent from system clock) and unit-tested carefully. It is easy to just write a few basic unit tests, and I could literally come up with failing tests in a minute.

chrzaszcz · 2023-12-18T07:43:33Z

src/opuntia.erl

+new({MaximumTokens, Rate, TimeUnit})
+  when ?NON_NEG_INT(MaximumTokens), ?NON_NEG_INT(Rate), ?TU(TimeUnit) ->
+    #token_bucket_shaper{shape = {MaximumTokens, Rate, TimeUnit},
+                         available_tokens = 0,


This rings an alarm bell instantly, so I tested it:

23> f(Sh),Sh=opuntia:new({100000,10000,second}),opuntia:update(Sh,0). {{token_bucket_shaper,{100000,10000,second},0,-576460334},0} 24> f(Sh),Sh=opuntia:new({100000,10000,second}),opuntia:update(Sh,1). {{token_bucket_shaper,{100000,10000,second},0,-576460099},1}

And I was right - the delay kicks in instantly, and you need a second to unblock.
So to continue my review I needed to work around it:

available_tokens = Rate,

The idea is that this wasn't necessarily wrong but whether you want the shapers to start charged or uncharged, it's a matter of deciding the API. Easy to change, only tests need to be adjusted.

Charging them was just a workaround. the main issue here is that if unit is second, then for up to a second I cannot use any tokens even though e.g. after 500 ms I should have the bucket halfway charged. So the unit starts acting like the counter resolution, which IMO shouldn't be the case.

Hmm... good point, but, that's more or less the contract too and why I separated bucket size from the rate. In a way that's what we had implicitly in MongooseIM, we had for example 60k tokens (bytes) per second but recharging was happening in milliseconds resolution, up to a maximum of 60k tokens regardless of how much time it had passed.

Also, the delay should always be given in milliseconds, not in the resolution you requested, all BEAM timers work in milliseconds so that's why I chose that.

chrzaszcz · 2023-12-18T07:49:03Z

src/opuntia.erl

+    RoundedDelay = ceil(MaybeDelay),
+
+    NewShaper = Shaper#token_bucket_shaper{available_tokens = TokensAvailable,
+                                           last_update = Now + RoundedDelay + 1},


Oh, this looks incorrect instantly. You need to store the accumulated fraction, because otherwise it would block you right away. Let's see:

8> lists:foldl(fun(N, ShIn) -> {ShOut, 0} = opuntia:update(ShIn, N), ShOut end, opuntia:new({100000,10000,second}), [1,1]). ** exception error: no match of right hand side value {#token_bucket_shaper{shape = {100000,10000,second}, available_tokens = 0,last_update = -576460643}, 1}

This also breaks for millisecond because the artificial penalty of 1 is still too much:

31> lists:foldl(fun(N, ShIn) -> {ShOut, 0} = opuntia:update(ShIn, N), ShOut end, opuntia:new({100000,10000,millisecond}), [1,1]). ** exception error: no match of right hand side value {#token_bucket_shaper{shape = {100000,10000,millisecond}, available_tokens = 0,last_update = -576460202884}, 1}

Only microsecond would save the situation because the clock can actually tick in the meantime:

21> lists:foldl(fun(N, ShIn) -> {ShOut, 0} = opuntia:update(ShIn, N), ShOut end, opuntia:new({100000,10000,microsecond}), lists:duplicate(1000,1)). #token_bucket_shaper{shape = {100000,10000,microsecond}, available_tokens = 89991,last_update = -576460237904693}

It still often breaks before filling the bucket, but of course the delay of 1 microsecond might not be noticeable in practice.

34> lists:foldl(fun(N, ShIn) -> {ShOut, 0} = opuntia:update(ShIn, N), ShOut end, opuntia:new({100000,10000,microsecond}), lists:duplicate(9990,1)). #token_bucket_shaper{shape = {100000,10000,microsecond}, available_tokens = 99999,last_update = -576457771123282} 35> lists:foldl(fun(N, ShIn) -> {ShOut, 0} = opuntia:update(ShIn, N), ShOut end, opuntia:new({100000,10000,microsecond}), lists:duplicate(9990,1)). ** exception error: no match of right hand side value {#token_bucket_shaper{shape = {100000,10000,microsecond}, available_tokens = 0,last_update = -576457768622881}, 1} in function erl_eval:expr/6 (erl_eval.erl, line 498) in call from erl_eval:exprs/6 (erl_eval.erl, line 136) in call from lists:foldl_1/3 (lists.erl, line 1599)

For me the +1 is just wrong, and I would store the accumulated fractional error itself separately.

A draft of what I meant by fractions is below. It passed my quick test in the console, but of course unit tests (and proper tests) would show if it works:

--- a/src/opuntia.erl +++ b/src/opuntia.erl @@ -60,8 +60,9 @@ new(0) -> new({MaximumTokens, Rate, TimeUnit}) when ?NON_NEG_INT(MaximumTokens), ?NON_NEG_INT(Rate), ?TU(TimeUnit) -> #token_bucket_shaper{shape = {MaximumTokens, Rate, TimeUnit}, - available_tokens = 0, - last_update = erlang:monotonic_time(TimeUnit)}. + available_tokens = Rate, + last_update = erlang:monotonic_time(TimeUnit), + frac = 0.0}. %% @doc Update shaper and return possible waiting time. %% @@ -73,11 +74,12 @@ update(none, _TokensNowUsed) -> {none, 0}; update(#token_bucket_shaper{shape = {MaximumTokens, Rate, TimeUnit}, available_tokens = LastAvailableTokens, - last_update = LastUpdate} = Shaper, TokensNowUsed) -> + last_update = LastUpdate, + frac = Frac} = Shaper, TokensNowUsed) -> %% Time since last shape update Now = erlang:monotonic_time(TimeUnit), - TimeSinceLastUpdate = Now - LastUpdate, + TimeSinceLastUpdate = (Now - LastUpdate) + Frac, %% How much we might have recovered since last time AvailableAtGrowthRate = Rate * TimeSinceLastUpdate, @@ -102,5 +104,6 @@ update(#token_bucket_shaper{shape = {MaximumTokens, Rate, TimeUnit}, RoundedDelay = ceil(MaybeDelay), NewShaper = Shaper#token_bucket_shaper{available_tokens = TokensAvailable, - last_update = Now + RoundedDelay + 1}, + last_update = Now + RoundedDelay, + frac = RoundedDelay - MaybeDelay}, {NewShaper, RoundedDelay}. diff --git a/src/opuntia.hrl b/src/opuntia.hrl index aff644f..3ec40f1 100644 --- a/src/opuntia.hrl +++ b/src/opuntia.hrl @@ -2,9 +2,10 @@ -define(OPUNTIA, true). -record(token_bucket_shaper, { + frac :: float(), shape :: opuntia:shape(), available_tokens :: opuntia:tokens(), - last_update :: number() + last_update :: integer() }).

chrzaszcz

Looks good, I added a few comments.

chrzaszcz · 2023-12-19T16:47:34Z

test/opuntia_SUITE.erl

-    timer:sleep(DelayMs),
-    run_shaper(NewShaper, TokensLeft - TokensConsumed).
+
+success_or_log_and_retuns(true, _S, _P) ->


chrzaszcz · 2023-12-19T16:49:59Z

test/opuntia_SUITE.erl

+
+should_take_in_range(#{rate := Rate, start_full := false}, ToConsume) ->
+    ExpectedMs = ToConsume / Rate,
+    {ExpectedMs, ExpectedMs + 1};


Is it correct that this is returning floats?

Yes, we want to know with a lot o precision the range with a tolerance of only 1ms more, so no rounding.

chrzaszcz · 2023-12-19T17:12:49Z

src/opuntia.erl

+%% Number of tokens accepted per millisecond.
+
+-type bucket_size() :: non_neg_integer().
+%% Maximum capacity of the bucket regardless of how much time it passes.


Suggested change

%% Maximum capacity of the bucket regardless of how much time it passes.

%% Maximum capacity of the bucket regardless of how much time passes.

chrzaszcz · 2023-12-19T17:18:03Z

src/opuntia.erl

+  when ?NON_NEG_INT(MaximumTokens),
+       ?NON_NEG_INT(Rate),
+       MaximumTokens >= Rate,
+       ?TU(millisecond),


This check doesn't make much sense.

Artifact of removing all the other code, removing this too 👌🏽

NelsonVides added 2 commits December 15, 2023 22:16

Clean the way we run tests

bfa225b

Let PropER try to shrink, give common-tests assertions instead of failures that will crash when trying to print, have all properties run the same number of tests.

Add test to not return delay if no token was consumed

23a427e

chrzaszcz reviewed Dec 18, 2023

View reviewed changes

NelsonVides force-pushed the improvements branch 3 times, most recently from 8f9bbb8 to 83c6e8b Compare December 19, 2023 15:46

chrzaszcz approved these changes Dec 19, 2023

View reviewed changes

NelsonVides added 7 commits December 19, 2023 18:28

Test shapers now with shapes instead of rates

fa8dcfe

Favour cleanup_* instead of gc_* naming

3fbd96f

Reimplement using shapes

70910df

Test case where bucket starts charged

86af4c8

Change how printing works on test failure

6739ad3

Reimplement fixing roundings and supporting only millisecond

846b270

Add simple unit test that no unnecesary delay is imposed

a16788b

NelsonVides force-pushed the improvements branch from d7ed258 to a16788b Compare December 19, 2023 17:29

NelsonVides merged commit 532155b into main Dec 19, 2023
3 checks passed

NelsonVides deleted the improvements branch December 19, 2023 17:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improvements #2

Improvements #2

NelsonVides commented Dec 15, 2023 •

edited

Loading

codecov-commenter commented Dec 15, 2023

chrzaszcz left a comment •

edited

Loading

chrzaszcz Dec 18, 2023

NelsonVides Dec 18, 2023

chrzaszcz Dec 18, 2023

NelsonVides Dec 18, 2023

chrzaszcz Dec 18, 2023 •

edited

Loading

chrzaszcz Dec 18, 2023 •

edited

Loading

chrzaszcz left a comment

chrzaszcz Dec 19, 2023

chrzaszcz Dec 19, 2023

NelsonVides Dec 19, 2023

chrzaszcz Dec 19, 2023

chrzaszcz Dec 19, 2023

NelsonVides Dec 19, 2023

	%% Maximum capacity of the bucket regardless of how much time it passes.
	%% Maximum capacity of the bucket regardless of how much time passes.

Improvements #2

Improvements #2

Conversation

NelsonVides commented Dec 15, 2023 • edited Loading

codecov-commenter commented Dec 15, 2023

Welcome to Codecov 🎉

chrzaszcz left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrzaszcz Dec 18, 2023 • edited Loading

Choose a reason for hiding this comment

chrzaszcz Dec 18, 2023 • edited Loading

Choose a reason for hiding this comment

chrzaszcz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NelsonVides commented Dec 15, 2023 •

edited

Loading

chrzaszcz left a comment •

edited

Loading

chrzaszcz Dec 18, 2023 •

edited

Loading

chrzaszcz Dec 18, 2023 •

edited

Loading