Skip to content

chore: Minor rbac memory optimization #7391

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
May 3, 2023
Merged

chore: Minor rbac memory optimization #7391

merged 10 commits into from
May 3, 2023

Conversation

Emyrk
Copy link
Member

@Emyrk Emyrk commented May 3, 2023 •
edited
Loading

What this does

RBAC roles are returned from a function to handle organization dynamic roles. The downside to this is that every time Authorize() is called, we have to allocate the Roles for said user.

For most of our roles, the properties are static; owner, member, template/user-admin, and auditor. For these, we can share the same Role struct and values. This role is completely protected by the RBAC package, so no external packages can mutate these in any way.

This is a cheap win. Further reduction in allocations is an ongoing effort.

Metrics

These metrics come from the newly added StaticRoles benchmark case. This case uses all the static roles for the actor. In production the benefits will not be as much since users still have organization roles which have the allocation penalty on each call.

Before

BenchmarkRBACAuthorize/Admin-8              1694            731187 ns/op          252034 B/op       5413 allocs/op
BenchmarkRBACAuthorize/StaticRoles-8                1214            843701 ns/op          291152 B/op       6518 allocs/op

The Roles.Expand for the Subject struct:

         0   119.01MB (flat, cum) 32.58% of Total
         .          .     67:func (s Subject) regoValue() (ast.Value, error) {
         .    19.01MB     68:   subjRoles, err := s.Roles.Expand()

After

BenchmarkRBACAuthorize/Admin-8              1740            646229 ns/op          201718 B/op       4173 allocs/op
BenchmarkRBACAuthorize/StaticRoles-8                1417            802061 ns/op          209982 B/op       4511 allocs/op

The Roles.Expand for the Subject struct:

         0     6.50MB (flat, cum)  1.81% of Total
         .          .     67:func (s Subject) regoValue() (ast.Value, error) {
         .     1.50MB     68:   subjRoles, err := s.Roles.Expand()

Subject Caching

Subject.astValue() copies all the Role.astValue()s into a slice. So we still pay an allocation cost on each authorize for a subject. I added the ability to cache the subject's ast value, so now each http.Request only pays the allocation cost once.

All dbauthz.AsSystem use a cached Subject to prevent some allocs.

This is not as large of an optimization, but it does help. Code was added to cache this in the same way as the roles. The savings of this is larger for larger roles.

BenchmarkRBACAuthorizeGroups/ManyRolesGroupACL-8                     925           1217610 ns/op          393721 B/op       8773 allocs/op
BenchmarkRBACAuthorizeGroups/ManyRolesCachedSubjectGroupACL-8               1150           1017163 ns/op          298037 B/op       6493 allocs/op

BenchmarkRBACAuthorizeGroups/StaticRolesGroupACL-8                          1952            646081 ns/op          201405 B/op       4318 allocs/op
BenchmarkRBACAuthorizeGroups/StaticRolesWithCacheGroupACL-8                 1738            610832 ns/op          196676 B/op       4202 allocs/op

@Emyrk Emyrk requested review from kylecarbs and johnstcn May 3, 2023 16:29
@Emyrk Emyrk merged commit 3368b8b into main May 3, 2023
@Emyrk Emyrk deleted the stevenmasley/rbac_static_roles branch May 3, 2023 19:42
@github-actions github-actions bot locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@johnstcn johnstcn johnstcn approved these changes

@kylecarbs kylecarbs kylecarbs approved these changes

Assignees

@Emyrk Emyrk

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Emyrk @johnstcn @kylecarbs