URL:
https://github.com/SSSD/sssd/pull/999
Title: #999: Mem-cache fixes/improvements
sumit-bose commented:
"""
...
> For the other `/ 8` I would suggest to round up the data from
the config file to the next integer which can be devided by 8 to be on the safe side.
I think this was already implemented in `sss_mmap_cache_init()`:
https://github.com/SSSD/sssd/blob/3c10595659698c7d8976bcbed88b3696c222d86...
Please let me know if you see any reason to move (or duplicate?) this protection or you
think it is done wrong.
You are right, I only looked at the patches and wasn't aware that it is already
done.
> I think the number of expected objects might be a better option.
> ...
> If we have a tool to get statistics from the memory cache and this tool can show the
average payload (i.e. how many slot a cached entry is using) we might consider to make the
payload configurable as well, e.g. by enhancing the number of expected option so that the
value can be `number_of_expected_objects:payload`. But should should be added alter.
I understand your intention but IMO from user point of view this is unjustified
over-complication. And most probably will not work well "in the field"...
My reasoning is as follows:
There is just no such thing as "average payload". What I typically meet in my
experience, is that, for example, groups mem-cache may contain entries with members amount
ranging from 1 to 30..60k(!) (this is typical for integration with very large AD - exactly
the case when mem-cache is very important).
Of course we can calculate average (or weighted average) size but this doesn't make
any sense. It would rather do harm because space will be wasted for small entries and
estimation will be wrong anyway.
So while I understand your idea - "user knows size of own database in amount of
entries => let them configure size of the cache in amount entries" - I don't
think this is realistic approach.
What I offer instead is perhaps very blunt but IMO should work well: "user got
message in system log that mem-cache is overfill => increase size of the cache".
This is less than ideal solution and still needs improvements (we should report cache
overfill only if we have to erase _non expired_ entries) but good in the sense of
effort/result ration from my point of view (I mostly mean users here, not developers)
I agree about the payload, the only purpose is to have some relation between the number of
slots in the data table and the number of entries in the hash table and I think it is
really not needed to make it configurable I was just saying that we can in case there will
be a potential use-case. My main concern if the integer division, especially with 3 and 5.
It should not do much harm because it is only the size of the hash table but I still think
it is better to avoid it and using the number of entries as an option would make it a
multiplication in the data table size.
From developers perspective, I would rather go in totally different
way as opposite to "average payload" (btw, justification of those factors in the
comments in the source code is very loose). I would prefer to get rid of fixed slot size
completly (I must admit I didn't investigate performance impact yet but my gut feeling
is it should be ok). But this would be quite large refactoring and intention of this PR -
to resolve specific bug and to provide at least basic means to ajust cache for NSS
load...
Originally the slot size was 32bytes and the fixed slot size together with all the memory
alignment was use to make sure the a slot or 2 will fix into a CPU cache line. This was of
course broken by extending the slot size to 40. But I think the original idea was good and
in the long term it would be good to come back to 32bytes (or maybe 64bytes). I'm not
sure if a flexible slot size would have much benefits.
If you prefer the number of slots as a config option I would suggest to make the factor
between the data and the hash table size (aka "payload") either constantly 4 or
even 1. The latter will of course increase the hash table size but with the positive side
effect that there might be less collisions.
bye,
Sumit
I hope this makes sense. Please let me know what you think.
"""
See the full comment at
https://github.com/SSSD/sssd/pull/999#issuecomment-601231341