in mem storage: got rid of the namespace indirection

[alexsnaps]

Looks better in this screenshot, but I will flamegraph and test this a little further on linux

[chirino]

lgtm

[alexsnaps]

Some further testing, with no `variables`, i.e actually hitting the change! Not that much difference... With the newly added bench scenario: #346

But if I grow the namespaces to a 1000...

Then it's a regression!

Now, that's all still on MacOS, I'll test these more rigorously on Linux next...

[alexsnaps]

Also... if a "tree" (namespace 1 -> * Limits -> 1 Counter) seem to be faster... how would storing these in a BTreeMap work out... 🤔

1000 namespaces with 50 limits each:

10 namespaces with 50 limits each

Well much... better. Anyways, Imma stop here and run all these on Linux and flamegraph some of these, but some of this would absolutely apply to our distributed storage /cc @chirino

[didierofrivia]

❤️

[didierofrivia]

👍🏼

[alexsnaps]

Did some more and, by now, relatively extensive testing... having the Limits comparable and store AtomicExpiringValue in a BTreeMap is giving the best results in most of the cases. When you have Limits with many (in the 100s) of Conditions, performance degrades. Still need to look into that further... But here is the callstack for Hash based vs Cmp based when searching for the Counters:

Hash based: HashMap<Namespace, HashMap<Limit, AtomicExpiringValue>>

Cmp based: BTreeMap<Limit, AtomicExpiringValue>

Giving us this kind of results:

Memory/check_rate_limited_and_update/10 namespaces with 50 limits each with 10 conditions and 0 vari...
                        time:   [253.02 µs 254.21 µs 255.86 µs]
                        change: [-9.1814% -8.4315% -7.7729%] (p = 0.00 < 0.05)
                        Performance has improved.

Zooming out...

Right now the worst offender tho, is not where we are looking for, in which ever storage version here btw:

RateLimiter::counters_that_applies spends all its time clone() the Limits (and composing parts) to then re-clone again and map into Counters... I'll have a look at that next.

What's next?

🛑 This cannot be merged! ⚠️

As of today we use the JSON serialized form of Limits to look counters up in Redis... It is highly unlikely that switching from HashMaps for our Limit field to BTreeMap would preserve the order we obtained in our own sorting functions.

So we first need to find a backwards compatible way of doing this all (i.e. having the crate::storage::keys::* functions keep on relying on the previous behavior, while using the new representation in-memory)

Limiter::counters_that_applies

I think we can probably store Arc<Limit> and use these instead of a full blown HashSet<Limit> on each request, requiring the cloning of all the Limits.

[alexsnaps]

🛑 This cannot be merged! ⚠️

As of today we use the JSON serialized form of Limits to look counters up in Redis... It is highly unlikely that switching from >HashMaps for our Limit field to BTreeMap would preserve the order we obtained in our own sorting functions.

I'll break the format anyways with more PRs coming… which is why we are releasing Limitador v2, with that in mind, maybe we are ok merging this?

[didierofrivia]

🪜