perf: optimize ACL role lookups with Map indexes and caching#2983
perf: optimize ACL role lookups with Map indexes and caching#2983benjie merged 12 commits intographile:mainfrom
Conversation
Replace O(n) linear scans in getRole(), getRoleByName(), and expandRoles() with Map-based lookups. Add WeakMap caches keyed by introspection object for role-by-id, role-by-name, auth_members-by-member-id indexes, and expandRoles results. This significantly improves performance for schemas with many roles, where these functions are called repeatedly during introspection.
🦋 Changeset detectedLatest commit: ad91b8a The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
We use this patch in production. Without it, loading 11000 roles took between 20 and 30 minutes (depending on the CPU). With it - it takes just 5 seconds. |
925febd to
87e0672
Compare
benjie
left a comment
There was a problem hiding this comment.
This was great; thank you for raising it! I especially liked the full test suite you included!
I try to avoid WeakMap where I can, and in this case since we own the introspection object adding the lookups and caches as part of that object made sense - that way the entire thing can be garbage collected at once. (This did mean I had to rewrite your tests a little, the "unit-testy" / mocked nature meant that as I changed the infrastructure the tests needed updating - the new style should be more resilient.)
I differentiated "lookups" (objects we already have in memory, identified by a key) versus "caches" (where calculation work is required) and went ahead and pre-computed all the lookups since the cost of doing so is marginal. Caches remain evaluated "on demand".
I also removed expansions to the comments that talked about how a function works, since those would rapidly go out of date as we refactor the code. It's preferred to keep descriptions focused on the purpose of the function rather than its inner workings.
I'll probably follow this up with additional optimizations following this pattern.
Replace O(n) linear scans in
getRole(),getRoleByName(), andexpandRoles()with Map-based lookups. Add WeakMap caches keyed by introspection object for role-by-id, role-by-name,auth_members-by-member-id indexes, and
expandRolesresults. This significantly improves performance for schemas with many roles, where these functions are called repeatedly during introspection.Description
During PostGraphile introspection,
getRole(),getRoleByName(), andexpandRoles()are called repeatedly for every entity's ACL resolution. The original implementation usesArray.find()andArray.includes()which are O(n) per call, andexpandRoles()iterates the fullauth_membersarray for each role. For schemas with many roles this becomes a significant bottleneck.This PR introduces:
expandRoles)expandRoles(O(1) membership check instead ofArray.includes)expandRolescalls (the common path) via WeakMapAll caches use
WeakMap<Introspection, ...>so they are automatically garbage-collected when the introspection object is no longer referenced. The public API is unchanged.Performance impact
Significant improvement for schemas with many roles. Role lookups go from O(n) to O(1).
expandRolesavoids redundant full scans ofauth_membersand caches results for repeated calls with the same role.The overhead of building indexes is amortized over the many calls that use them during a single introspection cycle.
Security impact
None. This is a purely internal optimization with no changes to the public API or behavior.
Checklist
yarn lint:fixpasses.yarn testpasses.RELEASE_NOTES.mdfile (if one exists).Notes on checklist:
expandRoles,aclContainsRole, andresolvePermissions(37/37 pass), covering role inheritance, NOINHERIT, caching, and circular membership.RELEASE_NOTES.mddoes not exist forpg-introspection.