Store qualified counters in dedicated capacity limited cache #195
Conversation
didierofrivia
force-pushed
the
atomic-expiring-value
branch
from
8bc47ee to
dc98716
Compare
alexsnaps
force-pushed
the
memleak-fix
branch
4 times, most recently
from
5ca651e to
ac1e00b
Compare
|
These are all the changes... By default, this now tries to use 70% of the available memory at server startup to store "qualified counters", it'll emit a |
Codecov Report
@@ Coverage Diff @@
## main #195 +/- ##
==========================================
- Coverage 74.32% 73.15% -1.18%
==========================================
Files 30 30
Lines 4998 5048 +50
==========================================
- Hits 3715 3693 -22
- Misses 1283 1355 +72
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| @@ -246,7 +246,7 @@ mod tests { | |||
| vec!["app_id"], | |||
| ); | |||
|
|
|||
| let limiter = RateLimiter::default(); | |||
| let limiter = RateLimiter::new(10_000); | |||
There was a problem hiding this comment.
I'm missing how this work, but I don't understand why is this change needed? Shouldn't invoke the default set the expected cache_size?
There was a problem hiding this comment.
There is no more any implementation of Default for RateLimiter. I couldn't really make a case for it. Why would 10k be a valid value for something using the crate? I thought that I could do the magic of using the 70% within default() could be an option, but again... why would 70% be any more desirable than 10% in the context of the crate? So I decided to push this "up" in the server's code (and avoid adding a dependency to the crate too).
Finally, for some reason, adding a impl Default for RateLimiter that'd only be for config test somehow failed. So didn't investigate much and added the value in the tests themselves.
| fn guess_cache_size() -> Option<u64> { | ||
| let sys = System::new_with_specifics(RefreshKind::new().with_memory()); | ||
| let free_mem = sys.available_memory(); | ||
| let memory = free_mem as f64 * 0.7; |
There was a problem hiding this comment.
isn't the 70% too much (or too little xD)
There was a problem hiding this comment.
Well... this math is "wrong", the size of the entries don't account for the overhead of the memory required by the cache itself... but most importantly don't account for the size of the variables data that's on the heap... i.e. all the entries of name/value pairs... we need some overhead for that.
Finally the "rest" also needs memory, i.e. the actix & tonic & all other plumbing (if only storing the Limits themselves... So for tiny amounts of sys.available_memory() 70% might actually be way too eager, while for large ones, probably on the overly cautious end... Which is why this will trigger a WARN level log.
| value = counter.value(); | ||
| } | ||
| } else if let Some(limits) = limits_by_namespace.get(counter.limit().namespace()) { | ||
| if let Some(counter) = limits.get(counter.limit()) { |
There was a problem hiding this comment.
what we are finding here is the AtomicExpiringValue, do you think it makes sense to be explicit and rename counter with something like expiring_value ? I also understand it's a counter in the end
I don't think anything should be addressed, but feel free to disagree... One thing which might be useful is setting a hard limit on what we know would be limitador's memory requirement. But we don't as of now... I could open an issue for that so that we at some point measure and address that. But right now, 70% isn't any better or worse than the previously hardcoded value... it's as bad, just differently. But at least, now, someone can set it to something that makes sense to their use-case. |
@didierofrivia here is what I am thinking...