-
Notifications
You must be signed in to change notification settings - Fork 3.4k
policy: Reduce allocs when keeping track of owners #34692
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
policy: Reduce allocs when keeping track of owners #34692
Conversation
53a4733
to
0174f2b
Compare
/test |
0174f2b
to
512c729
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Two nitpicks left inline.
512c729
to
176137b
Compare
/test |
176137b
to
db9b949
Compare
Rebased to absorb the visibility annotation removal. |
/test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did a first pass and currently trying to better understand the scope of the new Set data structure. Left some questions inline.
I don't know why the "ready-to-merge" label has been added since I just commented without acking the PR. 🤔 |
db9b949
to
46f4441
Compare
/test |
ci-runtime hit by #35055 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this looks better. 🙏
I've take a deeper look at the last two commits and left some additional feedback.
MapStateEntry.Merge need not be exported. Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
MapState keeps separate maps for allows and denies. There is no need to delete a key with a known entry value from both maps, as we can observe if the entry is a delete or an allow entry. Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
Add new generic container type set.Set optimized for cases where there typically is only one value in the set. A singular member is stored separately, while sets with more members use a map to store them. Set is not a refecence value, like a map is, so whenever a Set is stored by value (e.g., in a map as a value rather than via a pointer), the value must be updated when the Set changes from holding n/one to multiple and from multiple to n/one values. Due to this added complexity the Set should only be used in cases where there typically is at most one value, and the associated reduction in allocations is critical for performance or for memory use. The singular value is stored by reference in order to avoid the need for an additional boolean field to tell if the singlular value member is occupied or not. This saves about 10% of memory due to the 'set.Set' itself being smaller and has roughly the same performance as an inline value, when tested for interface member types. Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
Use the new 'set.Set' to store owners in MapStateEntry. Extend MapStateEntry.String() to output the owners, dependents, and listener priority. This eases unit test debugging going forward. With this change the BenchmarkRegenerateCIDRDenyPolicyRules benchmark runs ~10% faster and uses ~15% less memory with ~9% less allocs than with the prior map only version. Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
46f4441
to
9143174
Compare
/test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! 💯
Store single owners directly in the
MapStateEntry
and switch to a map only when more than one owner is needed. This is done with the newifset.Set
.Instead of a map of MapStateOwner interfaces, the Owners member of MapStateEntry is now a
ifset.Set
, which can hold a single interface value directly, or a more in a map. The newifset.Set
type uses an unexportedmemberMap
type for its internal map, so the Set can hold any interface values (including empty interface valuenil
).With this change the
BenchmarkRegenerateCIDRDenyPolicyRules
benchmark runs ~11% faster and uses ~15% less memory with ~9% less allocs than with the prior map only version.