perf: Reuse the resolved effective ACL across the passes of a single request - #2182
Draft
jeswr wants to merge 3 commits into
Draft
perf: Reuse the resolved effective ACL across the passes of a single request#2182jeswr wants to merge 3 commits into
jeswr wants to merge 3 commits into
Conversation
…request A single authenticated GET/HEAD invokes the WebAclReader several times with the same requestedModes but different credentials: once for the authorization decision (sharing its (credentials, requestedModes) object pair with the WAC-Allow user-permission pass) and once more for the WAC-Allow public pass, which uses a fresh empty credentials literal and therefore misses the CachedHandler. Each missing pass re-walks the container hierarchy and re-reads and re-parses the same effective .acl, even though the resolved ACL is identical for all of them. Memoize the credential-independent part of ACL resolution (the effective-ACL existence walk plus the read and parse of the relevant authorization statements) for the duration of a single request, keyed by the requestedModes AccessMap object in a WeakMap. The per-credential permission evaluation still runs for every call against the shared, read-only parsed ACL, so the granted permissions and the resulting authorization decision are unchanged. The cache is request-scoped by construction: the ModesExtractor produces the requestedModes object exactly once per request and shares it across the passes, so a different request always carries a different key and can never hit an earlier request's entry, and the WeakMap entry is collected once the request's AccessMap is unreachable. This mirrors the request-scoping already used by CachedResourceSet. Failed reads are not memoized. Measured effective-.acl read+parse counts (WAC enabled): an authenticated GET drops from 2 to 1, while the unauthenticated GET and PUT stay at 1. Model: claude-opus-4-8 Provenance: Opus 4.8 (Fable unavailable) — re-review/upgrade candidate Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…gent webId) The new regression tests had two TS errors only surfaced by tsc -p test (the CI test-unit typecheck), not by build:ts (src-only): an AccessMap built with the wrong generic (IdentifierSetMultiMap<ResourceIdentifier> -> <AccessMode>), and an un-narrowed credentials.agent.webId access. Test-only; the fix is unchanged. Model: claude-opus-4-8 Provenance: Opus 4.8 (Fable unavailable) -- re-review/upgrade candidate Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The added JSDoc/comments were more verbose than the surrounding authorization source; condense to the load-bearing facts (request-scoped WeakMap keyed by the per-request requestedModes, failures not memoized). No behaviour change. Model: claude-opus-4-8 Provenance: Opus 4.8 (Fable unavailable) — re-review/upgrade candidate Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Member
|
Note/reminder (for myself mostly) that if this gets finished I'll probably have to also add these changes in https://github.com/CommunitySolidServer/policy-engine (which is used in v8), if possible. |
jeswr
commented
Jun 29, 2026
Comment on lines
+97
to
+100
| const promise = (async(): Promise<Map<Store, ResourceIdentifier[]>> => { | ||
| const aclMap = await this.getAclMatches(requestedModes.distinctKeys()); | ||
| return this.findAuthorizationStatements(aclMap); | ||
| })(); |
Contributor
Author
There was a problem hiding this comment.
Suggested change
| const promise = (async(): Promise<Map<Store, ResourceIdentifier[]>> => { | |
| const aclMap = await this.getAclMatches(requestedModes.distinctKeys()); | |
| return this.findAuthorizationStatements(aclMap); | |
| })(); | |
| const promise = this.getAclMatches(requestedModes.distinctKeys()).then(aclMap => | |
| this.findAuthorizationStatements(aclMap)); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🚧 DRAFT — for @jeswr to review first (opened by @jeswr's AI coding agent; please hold off reviewing until marked ready).
✍️ Description
A single authenticated
GET/HEADresolves and parses the target's effective.acltwice. TheWebAclReaderruns once for the authorization decision (shared with theWAC-Allowuser pass via the same cached(credentials, requestedModes)pair) and again for theWAC-Allowpublic pass, which builds a fresh empty-credentials object and so misses theCachedHandler— re-walking the container hierarchy and re-reading + re-parsing the same ACL.This memoizes the credential-independent part of ACL resolution (the existence walk + read + parse) for the duration of a single request, in a
WeakMapkeyed by the per-requestrequestedModesobject thatModesExtractorproduces once per request. Each pass still evaluates its own credentials against the shared parsed ACL, so the granted permissions and theWAC-Allowheader are byte-identical. The cache is request-scoped by construction (a different request has a different key) and so can never serve a stale ACL; failed reads are not memoized.Added a regression
describeblock toWebAclReader.test.tscovering the within-request reuse (fails onmain), the no-stale-cache guarantee across requests, and the no-memoize-on-failure path.📈 Performance
Effective-
.aclread+parse operations per request (WAC enabled), the load-independent metric asserted by the regression test:This is the load-independent evidence for a fewer-reads change. The per-request saving is one effective-
.aclexistence-walk + read + parse on authenticated GETs, so the wall-clock effect is modest. A quick back-to-back wall-clock A/B (baselinemainvs this branch, both WAC-enabled, same file backend, constant-WebID extractor soohacould drive authenticated load) did not produce a stable reading on the test box — the build-to-build delta was swamped by background load (1-min load oscillating ~1.6→8.8 mid-run, the faster build flipping run to run). A clean wall-clock confirmation needs a quiet box; the deterministic op-count above is the reproducible evidence.✅ PR check list
Before this pull request can be merged, a core maintainer will check whether