[thjaeckle]

What

Memoizes the per-command policy-tree walk in TreeBasedPolicyEnforcer so recurring authorization decisions become a hash lookup instead of a full tree traversal.

Why

A production JFR profile showed the policy-tree walk (visitTree) as a dominant things-service CPU hotspot — roughly 10–12% of things-service CPU and ~5.6% of overall Ditto-microservice fleet CPU. Every authorization check (hasUnrestrictedPermissions, hasPartialPermissions, getSubjectsWithPermission, getSubjectsWithPartialPermission) walks the entire policy tree on each call, and the (resource, subjects, permissions) tuples recur heavily across signals (the same subjects repeatedly access the same resources).

This mirrors the existing classifySubjects(resource, READ) memoization on PolicyEnforcer: TreeBasedPolicyEnforcer is effectively immutable after createInstance (its tree is built once and only read afterwards), and a policy change produces a brand-new instance via PolicyEnforcers.defaultEvaluator. So the cache lifetime equals the instance lifetime and needs no explicit invalidation.

What changed

Added per-instance ConcurrentHashMap memos inside TreeBasedPolicyEnforcer:

• Boolean permission checks share one map keyed by (ResourceKey, subject-id Set, Permissions, partial-flag) — the partial flag keeps unrestricted and partial checks distinct.
• Subject-collection results are keyed by (ResourceKey, Permissions). The immutable EffectedSubjects instance is shared directly; getSubjectsWithPartialPermission caches an immutable snapshot and returns a fresh mutable copy per call to preserve its original contract.
• The authorization-subject-id Set is computed once and reused as the memo key, avoiding the previous per-call HashSet rebuild on cache hits.
• Each memo is best-effort bounded (MAX_MEMO_SIZE) to stay memory-safe under pathological key diversity; above the cap, results are computed uncached.

Placing the memo on the enforcer itself (rather than the PolicyEnforcer wrapper) transparently benefits every caller across things, policies and gateway, all of which invoke these checks on the bare Enforcer.

Behavior & compatibility

• Verdicts are byte-for-byte identical to the non-memoized path.
• Thread-safe: ConcurrentHashMap with a get→compute→putIfAbsent helper; the computation only reads the immutable tree and never re-enters the map.
• Only private members and private nested value classes are added — no public API change (japicmp passes). Java-8 compatible (plain value classes; no records / Set.copyOf).
• Adds TreeBasedPolicyEnforcerMemoTest proving repeat calls match a cold enforcer and that distinct subjects / permissions / resources / check-kinds are never conflated. The full policies/model suite (1055 tests) passes.

[thjaeckle]

@hu-ahmed could you have a brief look on this optimisation as well?

[hu-ahmed]

Nice change, and the safety homework is genuinely done — I verified the parts that would matter most: no time-dependent evaluation anywhere in the tree enforcer (so frozen verdicts can't go stale), DefaultEffectedSubjects and ImmutablePermissions really are immutable so sharing the cached instance is safe, and getSubjectsWithPartialPermission correctly keeps its mutable-set contract. Full policies/model suite green for me too (1057/0), plus policies/enforcement (268/0). No correctness concerns — verdicts are identical to the non-memoized path.

Two things I'd like to see addressed.

1. A cache hit still does most of the work the memo was meant to skip

All four memoized methods build the resource pointer before consulting the memo, and the two permission checks additionally re-derive the subject-id set. The pointer isn't part of the key and is unused on a hit, and AuthorizationContext.getAuthorizationSubjectIds() already memoizes the ids (ImmutableAuthorizationContext:133-147) — so the stream-and-collect is redundant.

Measured per call (ThreadMXBean.getCurrentThreadAllocatedBytes, 200k iterations after 50k warmup):

hasUnrestrictedPermissions (memo hit)        as-is               with patch
10-label policy,  1 caller subject     351.0 ns / 1373 B     41.9 ns /  88 B
100-label policy, 1 caller subject     304.2 ns / 1385 B     51.0 ns / 121 B
100-label policy, 5 caller subjects    353.8 ns / 1528 B     61.2 ns / 152 B

Attributing the eager work:

createAbsoluteResourcePointer                 77-108 ns / ~730 B   <- unused on a hit
stream -> HashSet (current)                    51- 62 ns / ~385 B
ctx.getAuthorizationSubjectIds() (memoized)       4.5 ns /    0 B   <- already cached

The fix is to key the memo on the already-memoized ID list, and build both the resource pointer and the HashSet inside the supplier so they are paid only on a miss. The set does matter: CheckPermissionsVisitor.visitSubjectNode calls authSubjectIds.contains(...) once per subject node in the tree, so handing the visitor a List instead would turn the miss path from O(policy subjects) into O(policy subjects x caller subjects). That's 14 insertions / 12 deletions, full 1057-test suite still green — happy to push it.

One consequence worth naming: the key becomes order-dependent (list equality), so two contexts holding the same subjects in a different order get separate entries. Verdicts are unaffected and the order is stable per caller in practice, but the PermissionCheckKey javadoc currently advertises order-independence and would need rewording.

Minor, related: the code comment calls this eager work "cheap" — it measures ~1.1 KB of the ~1.4 KB allocated per hit.

2. authorization-memo-cache-max-size = 0 doesn't fully disable the memo

The configured value is only threaded through PolicyEnforcer:273. The other defaultEvaluator call sites (:132, :215, :242, :355, :382, :503) use the hardcoded DEFAULT_MAX_MEMO_SIZE.

:503 is the one that matters: it builds the namespace-filtered enforcer, which is what ThingEnforcerActor enforces against on the things-service hot path. So the documented escape hatch — "Set to 0 to disable entirely - then no ConcurrentHashMaps are allocated and every check walks the tree directly" — does not hold for the very instance an operator would most likely be trying to disable it on.

Separately, (int) authorizationMemoMaxSize at :273 wraps negative for a configured value above Integer.MAX_VALUE, silently disabling the memo instead of enabling a large one. Worth clamping.

[thjaeckle]

Both points confirmed and fixed in e083c61, with a JMH benchmark in 57e2aa0.

1. Eager work on the hit path. The memo now keys on the context's already-memoized ID list, and the resource pointer + subject-ID HashSet moved into the supplier (miss-only). Kept the HashSet for the visitor for exactly the reason you gave — CheckPermissionsVisitor:72 probes it once per subject node and the field is typed Collection<String>, so passing a List would have compiled silently and gone quadratic. PermissionCheckKey javadoc reworded for order-dependence; the "cheap" comment is gone.

Two additions to your patch: checkNotNull(authorizationContext, ...) stays eager, since the old comment also cited null-argument check ordering and the NPE message is part of the contract; and the javadoc now records that the key relies on ImmutableAuthorizationContext returning an unmodifiable list, given it is no longer a private copy.

2. = 0 not disabling the memo. Right diagnosis, and :503 was indeed the one that mattered — the adjacent comment already argued that child must inherit the parent's bounds. PolicyEnforcer now retains the configured bound and passes it to the filtered child next to readClassificationCacheMaxSize, and the cast is a saturating clamp. Three regression tests (configured bound, 0, saturation above Integer.MAX_VALUE) all fail without the fix.

Same JMH harness run back-to-back against both enforcer versions (100 labels, 1 caller subject, -prof gc):

metric	before	after
hasUnrestrictedPermissions hit	255.6 ns / 1280 B	57.7 ns / 120 B
getSubjectsWithPermission hit	142.6 ns / 808 B	50.8 ns / 104 B

Your B/op figures reproduced exactly. Miss paths are unchanged within noise, with a small allocation drop there too (3424 → 3280 B) since getAuthorizationSubjectIds no longer re-streams the context. AuthorizationVerdictMemoBenchmark makes all of this re-runnable and keeps the removed work measurable (formerlyEagerPerHitWork, 1096 B) as a guard against reintroducing eager work on the hit path.

One thing worth flagging: policy-enforcement-use-throughput-optimized-evaluator-enabled defaults to true (ditto-devops.conf:43, and FeatureToggle.resolveProperty defaults unset properties to true), so on defaults defaultEvaluator returns the trie evaluator — which never memoizes and ignores maxMemoSize. This memo only engages where that toggle is explicitly false, as in the deployment the JFR came from. Relevant if you tried measuring locally and saw no effect.

policies/model 1058 and policies/enforcement 272 tests green, japicmp clean.

[hu-ahmed]

LGTM!
Thanks for the improvements