throttler icon indicating copy to clipboard operation
throttler copied to clipboard

Rate calculation is wrong when tens digit and unit digit aren't 0 (under 'second' unit)

Open visibletrap opened this issue 5 years ago • 1 comments

For example, for (throttle-fn f 140 :second), I get 100 req/s and for (throttle-fn f 150 :second), I get 200 req/s. I believe it's related to sleep-time and token-value calculation in chan-throttler*

  • For (throttle-fn f 140 :second), sleep-time is 10, token-value is 1 from (round (* 10 (/ 140 1000))). Total req per second is (* (/ 1000 sleep-time) token-value) which is 100 req/s
  • For (throttle-fn f 150 :second), sleep-time is 10, token-value is 2 from (round (* 10 (/ 150 1000))). Total req per second is (* (/ 1000 sleep-time) token-value) which is 200 req/s

The relationship between input req/s and actual req/s can be demonstrated with this function.

(defn- round [n] (Math/round (double n)))

(defn real-rate-s [rate-s]
  (let [rate-ms (/ rate-s 1000)
        min-sleep-time 10
        sleep-time (max (/ rate-ms) 10)
        token-value (round (* sleep-time rate-ms))]
    (* (/ 1000 sleep-time) token-value)))
(map (juxt identity real-rate-s) [40 50 140 150 240 250 940 950 1040 1050 1400 1500])
=> ([40 40N]
 [50 50N]
 [140 100]
 [150 200]
 [240 200]
 [250 300]
 [940 900]
 [950 1000]
 [1040 1000]
 [1050 1100]
 [1400 1400]
 [1500 1500])

As you can see, the inaccurate happens when tens digit and unit digit aren't 0.

Input 150 but get 200 is about 33% higher which is significant. I understand the limitation with min-sleep-time and token-value can't be decimal so I'm not sure what's a solution for this. Maybe just only a warning messing in README?

visibletrap avatar May 16 '19 09:05 visibletrap

I've been looking into this and I suspect the 10ms limit is from TIMEOUT_RESOLUTION_MS in core.async/timeout's implementation. There's a write-up on it here but basically timeout returns an existing channel for calls within 10ms of each other so they don't have to track as many timeouts.

We could probably re-implement core.async/timeout here and either drop the bucketing or make it configurable. I'm not sure that's the correct solution but I don't have any other reasonable ideas.

liath avatar May 22 '19 19:05 liath