perf(core): cache generated JSON schemas to remove lock from hot path

[jnduan]

Context

Follow-up to #2796 (now merged as 6ba5fef), which made JsonSchemaUtils
thread-safe by serializing every schema generation on SCHEMA_LOCK.

That fix is correct, but it re-runs reflective schema generation on every
structured-output and tool call: ReActAgent.doStructuredCall regenerates the
schema for the same class on each call, so under load the lock is taken
repeatedly for identical, deterministic work.

What this PR does

Cache the generated schema per Class / Type, so SCHEMA_LOCK is taken
only on a cache miss (the first time a class or type is seen). A cache hit
converts a fresh, independently mutable Map from the cached, never-mutated
node — no lock required.

Entries are held in ClassValue slots, so a slot lives on the class it
describes instead of in a static map that strongly references the class as a
key. The Type cache groups entries by their raw class
(ClassValue<Map<Type, JsonNode>>), so the parametrizations of one class share
a slot and follow class unloading.

Why it's safe

• Independent maps per call. Each call still returns a freshly-converted
  mutable Map, so callers that mutate the result in place (e.g.
  ToolSchemaGenerator, which hoists $defs out of nested schemas) cannot
  corrupt the cache or interfere with one another. Covered down to nested
  structures, not just the top level.
• No classloader pinning. Entries live on the class they describe rather
  than in a static Map<Class, JsonNode>, which strongly references the key
  class and therefore pins it — and its classloader — for the JVM's life. That
  matters because structured-output and tool-parameter classes are not always
  compile-time-fixed: agentscope-extensions loads skills and tools at
  runtime. Scoping entries to the class removes that edge rather than capping it.
• No computeIfAbsent. It runs the generation while holding the map's bin
  lock, which serializes unrelated classes hashing to the same bin behind a
  whole generation, and violates the contract that a remapping function must not
  modify its own map. Both caches now publish with a re-read inside
  SCHEMA_LOCK, which also preserves "generated exactly once per key".
• No deadlock. Lock order is one-directional (cache slot read →
  SCHEMA_LOCK); while holding SCHEMA_LOCK we only call generateSchema,
  never touching the cache slots, so there is no reverse path.
• Null behavior unchanged. A null argument still surfaces an NPE, matching
  the pre-cache path.

Known boundary: a dynamically loaded class appearing as a type argument (e.g.
List<TenantModel>) is still held by List's slot for as long as List lives,
because the inner map's keys are the parametrized types themselves. Closing
that would cost a lock on the hit path, or permanent misses with weak keys.
Types with no raw class (type variables, wildcards, generic arrays) are
generated without caching.

Tests

• Repeated calls return equal but independently mutable maps, for both Class
  and Type entry points, including mutations below the top level.
• Null-argument parity and the no-raw-class fallback are covered.
• The existing 12-thread concurrency tests from fix(core): make JsonSchemaUtils schema generation thread-safe #2796 still pass against the cache.
• Full agentscope-core suite: 2342 tests, 0 failures, 9 skipped.

No public API signature changes.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[oss-maintainer]

Summary

Caching the generated JsonNode per class/type and taking SCHEMA_LOCK only on a miss is the right fix for the hot-path serialization, and the "every call converts a fresh mutable Map" invariant is backed by tests. Filed as COMMENT rather than APPROVE: two robustness points are worth settling before merge (locks nested inside computeIfAbsent, and an unbounded static cache keyed by Class); neither is a correctness bug on today's call sites.

Findings

• [Warning] JsonSchemaUtils.java:135 — remapping function blocks on a global lock while the CHM bin lock is held; breaks the "must not mutate this map" contract and blocks unrelated keys in the same bin.
• [Warning] JsonSchemaUtils.java:92 — static, never-evicted cache with strong Class keys pins classloaders; unsafe under runtime-loaded skills / multi-tenant distribution.
• [Info] JsonSchemaUtilsTest.java:182 — independence tested only at the top level; nested $defs/properties mutation is what callers actually do.

Suggestions

Move to get -> double-check under lock -> put, and to ClassValue (or a bounded cache) for the key space. Both keep the public API and the existing tests green.

---

Automated review by github-manager-bot

[oss-maintainer]

computeIfAbsent runs the remapping function while holding the map's bin lock, and here that function then blocks on the process-wide SCHEMA_LOCK. Two consequences: (1) ConcurrentHashMap forbids a remapping function that mutates the same map — if anything reachable from schemaGenerator.generateSchema(...) (a Jackson module/serializer callback, or a future nested generateSchemaFromType call) touches this map, the second update blocks on the bin lock held by the first: self-deadlock on the same key, IllegalStateException on the same bin. (2) Unrelated classes hashing into the same bin are blocked for the whole generation, so the lock is only truly off the path on cache hits. Equivalent shape without nesting the locks:

JsonNode cached = CLASS_SCHEMA_CACHE.get(clazz);
if (cached == null) {
    synchronized (SCHEMA_LOCK) {
        cached = CLASS_SCHEMA_CACHE.get(clazz);
        if (cached == null) {
            cached = schemaGenerator.generateSchema(clazz);
            CLASS_SCHEMA_CACHE.put(clazz, cached);
        }
    }
}

TYPE_SCHEMA_CACHE (line 177) has the same issue.

[oss-maintainer]

The key is a strong Class<?> reference held in a static map that is never evicted, so each entry pins the class and its classloader for JVM lifetime. The comment argues the key space is bounded by compile-time-fixed classes, which holds for ReActAgent.generateSchemaFromClass(targetClass) and reflective @ToolParam types — but not for the paths this framework itself supports: agentscope-extensions loads skill/tool classes at runtime and agentscope-distribution runs multi-tenant, so per-tenant/per-skill classloaders mint fresh keys with no upper bound. That turns a hot-path optimization into a classloader leak that survives tenant unload. ClassValue<JsonNode> fits the class-keyed cache exactly (identity-scoped, classloader-safe, no eviction bookkeeping); the Type-keyed one needs a bound (e.g. Caffeine maximumSize with soft values). Worth one javadoc sentence naming which callers may pass dynamically loaded classes.

[oss-maintainer]

Independence is only asserted at the top level (first.put("description", ...)). The real mutating callers reach into nested structures — ToolSchemaGenerator/ReActAgent hoist $defs/definitions out of inner schemas and merge them into the params root, removing and replacing nested maps. Please add one case that mutates a nested entry (e.g. remove a key under properties, or perform a $defs hoist) and re-reads from a second call. Jackson's convertValue deep-copies, so the code is almost certainly fine — but that deep-copy is exactly the invariant this cache now silently depends on.

[jnduan]

Thanks for the review — both findings hold up, and I've reworked the caching along those lines.

1. computeIfAbsent / nested lock

My "no deadlock" reasoning was too narrow. I checked lock ordering (bin lock → SCHEMA_LOCK) and that SCHEMA_LOCK never touches the cache map, but missed the two points that make computeIfAbsent the wrong tool here:

• ConcurrentHashMap forbids the remapping function from modifying its own map. Nothing does that today, but the contract only holds by accident of what generateSchema currently reaches.
• Unrelated classes hashing to the same bin were serialized behind the whole generation — a real regression on the hot path, not just a theoretical one.

computeIfAbsent is gone. Both caches now re-read the slot inside the lock:

JsonNode cached = slot.get();
if (cached != null) {
    return cached;
}
synchronized (SCHEMA_LOCK) {
    cached = slot.get();              // double-check
    if (cached == null) {
        cached = schemaGenerator.generateSchema(type);
        slot.put(type, cached);
    }
}
return cached;

(The class path is the same shape, with an AtomicReference slot instead of a map.)

2. Classloader pinning

Adopted ClassValue for both caches: entries are held on the class they describe rather than in a static map that strongly references the class as a key, so the pinning edge is removed rather than capped.

One detail worth calling out: I do not generate inside ClassValue#computeValue. Its javadoc allows the JDK to invoke it more than once for the same class ("several racing threads have computed values, one is chosen"), which would re-generate whenever several threads first reach a class at once — a step back from the current guarantee of generating exactly once per key. So computeValue only allocates an empty slot (ClassValue<AtomicReference<JsonNode>>), and the value is published by the double-checked read above. Net: the same once-per-key guarantee as before, plus the classloader-safe scope.

For the Type cache I grouped keys by raw class — ClassValue<Map<Type, JsonNode>>, the inner map holding the parametrizations of one raw class (List<String> vs List<Integer>). Those entries are classloader-scoped in the same way, and unrelated raw classes no longer share a lock. Types with no raw class (type variables, wildcards, generic arrays) now generate without caching: correctness is unchanged, they only give up the caching benefit. That is a deliberate behavior difference from this PR's first revision, which cached them in the Type-keyed map.

That also made a separate bound unnecessary: the key space now follows class unloading instead of growing for JVM life, and agentscope-core has no Caffeine today, so I kept the dependency set untouched.

One boundary I did not close: the inner map's keys are the parametrized types themselves, so a dynamically loaded class appearing as a type argument (e.g. List<TenantModel>) is still held alive by List's slot for as long as List lives. Closing that too would need the inner map to track type-argument classes as well, at the cost of taking a lock on the hit path — or of never hitting, if keys were weak, since reflection hands out a fresh Type instance per call. Since the extension paths in the finding load skill/tool classes as the target class itself, I went with the raw-class grouping and left that corner open. Happy to revisit if you'd rather close it.

3. Nested mutation coverage

Added two cases that mutate below the top level — removing a key under properties, and writing into a nested property schema — then read the same class/type again and assert the pristine shape. This pins down the deep-copy invariant that ToolSchemaGenerator's $defs hoisting depends on. Also added a case for the no-raw-class fallback above, and updated the null-argument comment that still referred to computeIfAbsent.

Verified: spotless:check clean, JsonSchemaUtilsTest 16/16, full agentscope-core suite 2342 tests / 0 failures / 9 skipped.

Please take another look.

[oss-maintainer]

Summary

Replaces the two static ConcurrentHashMap schema caches with ClassValue slots, so a cached schema is held on the class it describes instead of being strongly referenced by a static map key. That stops an entry from keeping a class — and its defining classloader — reachable, which is the right fix for the runtime-loaded-skill and per-tenant-classloader cases. Left COMMENT over the uncached-fallback cost and the unbounded per-class Type map; both are judgement calls, not bugs.

The double-checked read is correct: AtomicReference and ConcurrentHashMap give the needed visibility, the slot is re-read under SCHEMA_LOCK so a contended class is generated exactly once, and the change also removes the pre-existing hazard of running a long lock-holding mapping function inside ConcurrentHashMap.computeIfAbsent's bin lock. The new nested-mutation tests are a genuinely valuable addition — testGenerateSchemaFromClassNestedMutationDoesNotAffectLaterCalls pins the nested copy invariant the whole cache rests on, where the previous tests only checked the top level.

Findings

• [Warning] JsonSchemaUtils.java:236 — types with no raw class (type variables, wildcards, generic arrays) now regenerate under the global SCHEMA_LOCK on every call, where the old Map<Type, JsonNode> cached them. Correctness is covered by testGenerateSchemaFromTypeVariableStillGenerates; cost is not asserted anywhere. Because this path holds the process-wide lock for a full generation, it also stalls schema generation for unrelated classes. Keying a small ConcurrentHashMap<Type, JsonNode> fallback off the Type itself is cheap and removes the cliff; if instead you can confirm no reachable structured-output path produces such a Type, say so in the javadoc so the next reader does not have to re-derive it.
• [Warning] JsonSchemaUtils.java:116 — TYPE_SCHEMA_SLOT is unbounded and unevicted, and the original "keys are compile-time-fixed classes" bound argument no longer holds for it: entries are keyed by Type variant within a raw class, so a commonly-parameterized raw class (List, Map, Optional) accumulates one entry per distinct generic signature ever seen. In exactly the multi-tenant / dynamically-loaded scenario that motivates this PR, those signatures may be produced per tenant or per runtime-loaded skill. The leak direction is improved; the per-class entry count is the new unbounded dimension and deserves either a stated bound or a documented rationale.
• [Info] JsonSchemaUtils.java:268 — the public null contract of generateSchemaFromType is now enforced by a private helper's exception. Objects.requireNonNull(type, "type") as the first statement of the public method expresses the same guarantee where a caller reading the signature would look.

CI note

build (windows-latest) is failing and build (ubuntu-latest) is CANCELLED. The windows failure is LocalFilesystemPersonalAssistantExampleTest.localFilesystem_filesPersistAcrossCalls:103 ("MEMORY.md content should be persisted on disk ==> expected: but was: ") in agentscope-harness — untouched by this diff, and recent main runs are green, so it reads as a windows-specific flake rather than a regression here. Still needs a green or maintainer-explained build before merge; and since this PR is specifically about lock contention, the CANCELLED ubuntu job is worth a re-run rather than ignoring.

Tests

A concurrency test exercising generateSchemaFromType on two variants of the same raw class would document the shared-map behaviour the new javadoc describes.

---

Automated review by github-manager-bot

[oss-maintainer]

[Warning] Uncacheable types now regenerate on every call while holding the global SCHEMA_LOCK.

Type variables, wildcards and generic arrays have no raw class to hang a slot on, so they bypass caching entirely and take the process-wide lock for the full duration of schemaGenerator.generateSchema(type). Previously TYPE_SCHEMA_CACHE was a plain Map<Type, JsonNode> and cached these types like any other, so this is a latency regression on the fallback path — and because it holds the shared lock, it also stalls schema generation for every other class in the JVM.

The added test (testGenerateSchemaFromTypeVariableStillGenerates) proves the path is correct but asserts nothing about cost. Two options:

1. key a second ClassValue-free ConcurrentHashMap<Type, JsonNode> fallback off the Type itself (these types are interned/equals-stable, so they are fine as keys), or
2. confirm with a benchmark that no reachable structured-output path produces a TypeVariable/WildcardType, and say so in the javadoc so the next reader does not have to re-derive it.

Option 1 is cheap and removes the cliff.

[oss-maintainer]

[Warning] The TYPE_SCHEMA_SLOT map has no size bound or eviction, and the javadoc's original bound argument no longer applies to it.

The old CLASS_SCHEMA_CACHE javadoc justified being unbounded because keys were compile-time-fixed classes. Entries here are now keyed by Type variant within a raw class, so a raw class that is commonly parameterized (List, Map, Optional) accumulates one entry per distinct generic signature ever passed to generateSchemaFromType. In the multi-tenant / runtime-loaded scenarios this PR explicitly motivates — where the classloader-leak fix is the whole point — those signatures can be produced per tenant or per dynamically loaded skill rather than being fixed at compile time.

The leak direction is genuinely improved (a slot no longer pins its class), but the per-class entry count is now the unbounded dimension. Worth either a stated bound or a sentence in the javadoc recording that this is deliberate and why the variant count stays small in practice.

[oss-maintainer]

[Info] rawTypeOf throws NullPointerException for a null argument, and generateSchemaFromType(null) relies on that to keep the pre-cache contract (covered by testGenerateSchemaFromTypeNullThrows).

That works, but the public method's null contract is now enforced by a private helper's exception message ("type must not be null") rather than at the entry point. Objects.requireNonNull(type, "type") as the first statement of generateSchemaFromType expresses the same guarantee where a caller reading the public signature would look, and keeps the test from silently passing if the helper is ever inlined away.

[oss-maintainer]

Summary

Replaces the two static ConcurrentHashMap schema caches with ClassValue slots, so a cached schema is held on the class it describes instead of being strongly referenced by a static map key. That stops an entry from keeping a class — and its defining classloader — reachable, which is the right fix for the runtime-loaded-skill and per-tenant-classloader cases. Left COMMENT over the uncached-fallback cost and the unbounded per-class Type map; both are judgement calls, not bugs.

The double-checked read is correct: AtomicReference and ConcurrentHashMap give the needed visibility, the slot is re-read under SCHEMA_LOCK so a contended class is generated exactly once, and the change also removes the pre-existing hazard of running a long lock-holding mapping function inside ConcurrentHashMap.computeIfAbsent's bin lock. The new nested-mutation tests are a genuinely valuable addition — testGenerateSchemaFromClassNestedMutationDoesNotAffectLaterCalls pins the nested copy invariant the whole cache rests on, where the previous tests only checked the top level.

Findings

• [Warning] JsonSchemaUtils.java:236 — types with no raw class (type variables, wildcards, generic arrays) now regenerate under the global SCHEMA_LOCK on every call, where the old Map<Type, JsonNode> cached them. Correctness is covered by testGenerateSchemaFromTypeVariableStillGenerates; cost is not asserted anywhere. Because this path holds the process-wide lock for a full generation, it also stalls schema generation for unrelated classes. Keying a small ConcurrentHashMap<Type, JsonNode> fallback off the Type itself is cheap and removes the cliff; if instead you can confirm no reachable structured-output path produces such a Type, say so in the javadoc so the next reader does not have to re-derive it.
• [Warning] JsonSchemaUtils.java:116 — TYPE_SCHEMA_SLOT is unbounded and unevicted, and the original "keys are compile-time-fixed classes" bound argument no longer holds for it: entries are keyed by Type variant within a raw class, so a commonly-parameterized raw class (List, Map, Optional) accumulates one entry per distinct generic signature ever seen. In exactly the multi-tenant / dynamically-loaded scenario that motivates this PR, those signatures may be produced per tenant or per runtime-loaded skill. The leak direction is improved; the per-class entry count is the new unbounded dimension and deserves either a stated bound or a documented rationale.
• [Info] JsonSchemaUtils.java:268 — the public null contract of generateSchemaFromType is now enforced by a private helper's exception. Objects.requireNonNull(type, "type") as the first statement of the public method expresses the same guarantee where a caller reading the signature would look.

CI note

build (windows-latest) is failing and build (ubuntu-latest) is CANCELLED. The windows failure is LocalFilesystemPersonalAssistantExampleTest.localFilesystem_filesPersistAcrossCalls:103 ("MEMORY.md content should be persisted on disk ==> expected: but was: ") in agentscope-harness — untouched by this diff, and recent main runs are green, so it reads as a windows-specific flake rather than a regression here. Still needs a green or maintainer-explained build before merge; and since this PR is specifically about lock contention, the CANCELLED ubuntu job is worth a re-run rather than ignoring.

Tests

A concurrency test exercising generateSchemaFromType on two variants of the same raw class would document the shared-map behaviour the new javadoc describes.

---

Automated review by github-manager-bot

[oss-maintainer]

[Warning] Uncacheable types now regenerate on every call while holding the global SCHEMA_LOCK.

Type variables, wildcards and generic arrays have no raw class to hang a slot on, so they bypass caching entirely and take the process-wide lock for the full duration of schemaGenerator.generateSchema(type). Previously TYPE_SCHEMA_CACHE was a plain Map<Type, JsonNode> and cached these types like any other, so this is a latency regression on the fallback path — and because it holds the shared lock, it also stalls schema generation for every other class in the JVM.

The added test (testGenerateSchemaFromTypeVariableStillGenerates) proves the path is correct but asserts nothing about cost. Two options:

1. key a second ClassValue-free ConcurrentHashMap<Type, JsonNode> fallback off the Type itself (these types are interned/equals-stable, so they are fine as keys), or
2. confirm with a benchmark that no reachable structured-output path produces a TypeVariable/WildcardType, and say so in the javadoc so the next reader does not have to re-derive it.

Option 1 is cheap and removes the cliff.

[oss-maintainer]

[Warning] The TYPE_SCHEMA_SLOT map has no size bound or eviction, and the javadoc's original bound argument no longer applies to it.

The old CLASS_SCHEMA_CACHE javadoc justified being unbounded because keys were compile-time-fixed classes. Entries here are now keyed by Type variant within a raw class, so a raw class that is commonly parameterized (List, Map, Optional) accumulates one entry per distinct generic signature ever passed to generateSchemaFromType. In the multi-tenant / runtime-loaded scenarios this PR explicitly motivates — where the classloader-leak fix is the whole point — those signatures can be produced per tenant or per dynamically loaded skill rather than being fixed at compile time.

The leak direction is genuinely improved (a slot no longer pins its class), but the per-class entry count is now the unbounded dimension. Worth either a stated bound or a sentence in the javadoc recording that this is deliberate and why the variant count stays small in practice.

[oss-maintainer]

[Info] rawTypeOf throws NullPointerException for a null argument, and generateSchemaFromType(null) relies on that to keep the pre-cache contract (covered by testGenerateSchemaFromTypeNullThrows).

That works, but the public method's null contract is now enforced by a private helper's exception message ("type must not be null") rather than at the entry point. Objects.requireNonNull(type, "type") as the first statement of generateSchemaFromType expresses the same guarantee where a caller reading the public signature would look, and keeps the test from silently passing if the helper is ever inlined away.