Improve performance when requesting availableContentScopes - #5999
Improve performance when requesting availableContentScopes#5999fraxachun wants to merge 1 commit into
Conversation
|
Please add a description what this PR improves and by how much. And how caching (clearing) works. |
0859e39 to
342461e
Compare
| private availableContentScopesCache: ContentScopeWithLabel[] | undefined; | ||
|
|
||
| // Populates the cache used by getAvailableContentScopes() for the duration of one operation (e.g. resolving a list of users). | ||
| // Must be paired with clearAvailableContentScopesCache() so later, unrelated requests don't see a stale snapshot. |
There was a problem hiding this comment.
this cache isn't user specific, does it really need to be per request?
And if it is per request, you should use a Request scoped Service for that. The clear logic (called in a finally) doesn't work for multiple running requests, they all share a single service instance.
(also, a explicit warmupCache and clearCache isn't a easy to use api design)
There was a problem hiding this comment.
this cache isn't user specific, does it really need to be per request?
I assume that you never want to risk this cache to be outdated because scopes could be added dynamically. So only making it per request makes sense to me.
There was a problem hiding this comment.
It has to be per request. However, I refactored only this change into a data loader in a new PR: #6113
| "@comet/cms-api": patch | ||
| --- | ||
|
|
||
| Improve performance when requesting `availableContentScopes` |
There was a problem hiding this comment.
Would also be nice to have some more info here about what was actually changed
| private availableContentScopesCache: ContentScopeWithLabel[] | undefined; | ||
|
|
||
| // Populates the cache used by getAvailableContentScopes() for the duration of one operation (e.g. resolving a list of users). | ||
| // Must be paired with clearAvailableContentScopesCache() so later, unrelated requests don't see a stale snapshot. |
There was a problem hiding this comment.
this cache isn't user specific, does it really need to be per request?
I assume that you never want to risk this cache to be outdated because scopes could be added dynamically. So only making it per request makes sense to me.
VPS-Obi
left a comment
VPS-Obi
left a comment
There was a problem hiding this comment.
Loading 2000 content scopes per request is still a lot, do we want to support that many content scopes in the user permissions module?
We don't want to have 2000 content scopes. However, these changes are a quick win since this is a regression problem in the current comet version. Since there are applications with that many content scopes out there they are currently facing a performance problem when upgrading to new most recent comet version,. |
…ader (#6113) ## Problem When the admin queries the users list (`userPermissionsUsers`), the `contentScopesCount` field resolver runs **once per row**. Each invocation calls `getContentScopes(user)`, which recomputes the full list of available content scopes (build labels + `uniqWith` deduplication) from scratch. For a page of users this repeats the same shared computation N times, which becomes very slow when a project configures many content scopes. ## Solution Resolve the available content scopes **once per request** by routing `contentScopesCount` through a request-scoped `DataLoader` (`UserContentScopesLoaderService`), following the existing DataLoader pattern in cms-api (e.g. `AttachedDocumentLoaderService`). All users of a request are batched into a single `getContentScopesForUsers(users)` call that computes the available content scopes only once and then maps each user's scopes. This PR **keeps the existing `lodash.uniqWith` deduplication** unchanged — the only change is *how often* the available content scopes are computed. ## Speed comparison (25 users) Measured by resolving `contentScopesCount` for 25 users (median of 3 runs), comparing the current per-row behavior against the DataLoader: **Typical case — each user resolves to a few content scopes** | Available content scopes | Current (per row) | DataLoader | Speedup | | ------------------------ | ----------------- | ---------- | ------- | | 250 | 359 ms | 15 ms | ~25× | | 500 | 1 377 ms | 55 ms | ~25× | | 1 000 | 5 400 ms | 209 ms | ~26× | | 2 000 | 20 738 ms | 821 ms | ~25× | **Worst case — every user resolves to *all* content scopes** (the per-user `uniqWith`, which this PR does not change, then dominates in both variants): | Available content scopes | Current (per row) | DataLoader | Speedup | | ------------------------ | ----------------- | ---------- | ------- | | 250 | 558 ms | 268 ms | ~2.1× | | 500 | 2 229 ms | 998 ms | ~2.2× | | 1 000 | 9 305 ms | 4 075 ms | ~2.3× | | 2 000 | 35 057 ms | 15 771 ms | ~2.2× | So the DataLoader removes 24 of 25 redundant available-content-scope computations per request; the remaining cost is the per-user work, which is unchanged. ## Example / test `user-permissions.service.spec.ts` covers the batching guarantee (available content scopes computed only once for all users) and that each user's scopes are returned in input order. ## Notes Supersedes #5999 (which additionally replaced the `uniqWith` deduplication with a custom algorithm). This PR isolates just the DataLoader change. https://claude.ai/code/session_01RGR2hkqpqcXL4xk7rGqz7n Co-authored-by: Claude <noreply@anthropic.com>
4 participants
The
availableContentScopeswere repeatedly recomputed for every row when querying users; this is fixed by the cache (per resolver request). Additionally, filtering out duplicate content scopes was very slow when usinglodash.uniqWithwith many content scopes, and was significantly sped up by implementing a custom algorithm (on my machine, with 2000 content scopes, from about 15 seconds down to 0.15 seconds)."