rolling window rate limiter #25423
rolling window rate limiter #25423
Conversation
04c051d to
e044217
Compare
e044217 to
21a8113
Compare
|
Hi @caoilainnl |
21a8113 to
d08ed7a
Compare
made the queue thread safe
021ac92 to
aad1d09
Compare
|
@caoilainnl impressive work 👏 |
|
|
||
| private bool running = false; | ||
| private List<(long timestamp, double cost)> timestamps = new List<(long, double)>(); |
There was a problem hiding this comment.
@carlosmiei what about having ConcurrentList here instead of the regular list? or SlimList even?
There was a problem hiding this comment.
I might have to look into this more, I've read a little bit about it and it looks like right now, timestamps is single-threaded, so a plain List is fine (and slightly faster). Switching to a concurrent collection should only happen if the list is to run outside rollingWindowLoop() as well (which I don't think it ever would)
There was a problem hiding this comment.
@carlosmiei what about having ConcurrentList here instead of the regular list? or SlimList even?
I think it's better to use a regular list. I haven't run into concurrency issues using it yet, and it looks like a plain List is slightly faster
There was a problem hiding this comment.
If it is needed, I created thread safe timestamps on caoilainnl@d1feae0 , I don't think it is though because theres only 1 iteration of the rollingWindowLoop executing at a time, and the timestamp list isn't accessed anywhere else (other than NewThrottler, which is only called once)
There was a problem hiding this comment.
@carlosmiei do you think a thread safe timestamp list would be useful?
…lainnl/ccxt into rolling-window-rate-limiter
Co-authored-by: T. Todua <7117978+ttodua@users.noreply.github.com>
Co-authored-by: T. Todua <7117978+ttodua@users.noreply.github.com>
Co-authored-by: T. Todua <7117978+ttodua@users.noreply.github.com>
Co-authored-by: T. Todua <7117978+ttodua@users.noreply.github.com>
…cxt into rolling-window-rate-limiter
| @@ -1619,6 +1619,7 @@ export default class bitget extends Exchange { | |||
| // fiat currencies on deposit page | |||
| 'fiatCurrencies': [ 'EUR', 'VND', 'PLN', 'CZK', 'HUF', 'DKK', 'AUD', 'CAD', 'NOK', 'SEK', 'CHF', 'MXN', 'COP', 'ARS', 'GBP', 'BRL', 'UAH', 'ZAR' ], | |||
| }, | |||
| 'rollingWindowSize': 1000, | |||
There was a problem hiding this comment.
| 'rollingWindowSize': 1000, | |
| 'rateLimiterAlogrithm': 'leakyBucket', // `leakyBucket` or `rollingWindow` | |
| 'rollingWindowSize': 1000, |
I meant this
There was a problem hiding this comment.
I don't see any benefit to this. The only reason it should be added to an exchange would be if rollingWindow caused the rateLimiter to go over (because the exchange tracked requests using leakyBucket or something).
I included rollingWindowSize on Bitget here because bitget can only consume a maximum per second, but with Binance it was a maximum weight per minute
There was a problem hiding this comment.
@caoilainnl,
last nuance - even if you set that, how do you tell exchange to use rollingWindow algorithm? I mean, some extra check is needed in base. telling users to do new bitget({'rateLimiterAlgorithm':'rollingWindow'}) does not seem a good way.
@carlosmiei wdyt?
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
@caoilainnl, last nuance - even if you set that, how do you tell exchange to use
rollingWindowalgorithm? I mean, some extra check is needed in base. telling users to donew bitget({'rateLimiterAlgorigthm':'rollingWindow'})does not seem a good way.@carlosmiei wdyt?
You're saying enable it by default if rollingWindowSize has been assigned? That could probably be done, unless there's a reason not to
@carlosmiei can you think of a reason not to do this?
A rolling window rate limiter is more efficient because it reduces wait times when part of your rate limit is still available. It also allows an initial burst of requests.
Some exchanges, such as Bitget, use a window size of 1000 ms. This means 20 requests should be sent immediately in the first second, another 20 in the second, and so on. To ensure this rate limiter works with all exchanges, the rollingWindowSize property will need to be configured individually for each exchange.
It is also likely a good idea to keep the leakyBucket rate limiter available, since some exchanges track usage with a leaky bucket model, using only a rolling window limiter could otherwise result in going over their limits.
I’m using the following programs to test this: https://github.com/caoilainnl/test-rate-limiter
The results show significant time improvements. All tests were run on Binance.
Typescript
Python
PHP
C#
GO