[Pcgen-github] [PCGen/pcgen] 2f6b6a: perf: improve performance of isType/Type.getConsta...

[Vest <no...@gi...>]

  Branch: refs/heads/master
  Home:   https://github.com/PCGen/pcgen
  Commit: 2f6b6a0a3783c84276e811c2793c3feabfb9dd36
      https://github.com/PCGen/pcgen/commit/2f6b6a0a3783c84276e811c2793c3feabfb9dd36
  Author: Vest <Ve...@us...>
  Date:   2026-08-24 (Mon, 24 Aug 2026)

  Changed paths:
    M build.gradle
    A code/gradle/benchmark.gradle
    M code/src/java/pcgen/cdom/base/CDOMObject.java
    M code/src/java/pcgen/cdom/base/Loadable.java
    M code/src/java/pcgen/cdom/list/ClassSkillList.java
    M code/src/java/pcgen/cdom/list/ClassSpellList.java
    M code/src/java/pcgen/cdom/list/DomainSpellList.java
    M code/src/java/pcgen/cdom/reference/AbstractReferenceManufacturer.java
    M code/src/java/pcgen/core/AbilityCategory.java
    M code/src/java/pcgen/core/PObject.java
    M code/src/java/pcgen/util/Comparators.java
    A code/src/jmh/pcgen/cdom/enumeration/SortBenchmark.java
    A code/src/jmh/pcgen/cdom/enumeration/TypeBenchmark.java
    A code/src/test/pcgen/cdom/reference/AbstractReferenceManufacturerGroupResolutionTest.java

  Log Message:
  -----------
  perf: improve performance of isType/Type.getConstant and Collator on campaign load (#7711)

* perf: add PObject.isType(Type) fast path + JMH benchmark

PObject.isType(String) is the leaf dominating PCGen campaign-load CPU:
it uppercases, tokenizes, and re-interns via Type.getConstant on every
call. AbilityCategory.populate already holds interned Type constants but
was round-tripping them through toString() into that slow path.

Add PObject.isType(Type) that goes straight to containsInList(ListKey.TYPE,
type), and switch AbilityCategory.populate to it. Behaviour-preserving:
containsInList compares Type by value-equals, identical to what the String
path produces after interning.

Add a JMH benchmark covering the hot leaf, wired via a separate
code/gradle/benchmark.gradle (new `jmh` source set + `benchmark` task),
following the existing apply-from convention for developer-only tasks.
Measured (avgt, ns/op): single-token 25.8 -> 3.0, multi-token 98.6 -> 7.8.

resolveGroupReferences (the other hot caller) is deliberately left on the
String path: it dispatches isType polymorphically and Equipment.isType has
divergent (EqMod-merged) semantics, so a Type fast path there needs a
proper Loadable.isType(Type) design, not a shortcut.

* perf: fast-path resolveGroupReferences via Loadable.isType(Type)

resolveGroupReferences called obj.isType(String) for every
object x typeReference x token, re-interning each token through
Type.getConstant on every call — the remaining dominant consumer of that
leaf in the campaign-load profile after the AbilityCategory.populate fix.

Add isType(Type) as a default method on Loadable that delegates to
isType(String), so all ~30 implementers keep their exact current behaviour
by default — critically Equipment, whose isType merges EqMod-derived types
and must not use the base containsInList path. Override the default only
where provably identical: PObject (containsInList(ListKey.TYPE, type)) and
ClassSkillList/ClassSpellList (types.contains(type)).

resolveGroupReferences now interns each group key's tokens once, before the
object loop (getTypeReference guarantees single plain tokens), then calls
isType(Type) per object. Turns objects x groups x tokens interns into
groups x tokens.

Benchmark (avgt, 50 objects x 2 tokens): groupResolve 2989 -> 747 ns/op
(~4x). Guardrails green: Pre{Type,ArmorType,SpellType,Charactertype}
RoundRobin, PObjectTest, PreArmorTypeTest.

* perf: share one Collator per name-sort comparator instead of per compare

CDOMObject.P_OBJECT_NAME_COMP and Comparators.TreeTableNodeComparator each
called Collator.getInstance() inside compare(), constructing a Collator on
every comparison (O(n log n) per sort) — the DataSet.initLists ->
sortPObjectListByName tower in the campaign-load profile.

Hoist the Collator to a shared private static final field, matching the
existing pattern in Comparators.ToStringIgnoreCaseCollator. Sort logic is
unchanged; DataSetTest and PObjectTest confirm identical order.

The win is allocation, not CPU: the JDK caches Collator rule data, so
getInstance() is a cheap clone rather than a recompile — wall time moved
only ~5% (within noise). But it cuts allocation materially: JMH -prof gc on
a 200-element sort shows 1,870,127 -> 1,315,870 B/op (~30% less garbage),
which matters during startup when many lists sort concurrently with other
init work. SortBenchmark documents both comparators for future runs.

* build: drop JavaFX modules from the benchmark task

The JMH benchmarks exercise only core data-path classes
(PObject/CDOMObject/Type), none of which load JavaFX, so the JavaFX
module path and the extractJavaFXLocal dependency are unnecessary.
Remove them (keeping only -Djava.awt.headless=true).

* refactor: eager-init type sets in Class/Domain spell & skill lists

ClassSkillList, ClassSpellList and DomainSpellList lazily created their
`types` set on first addType and null-guarded every read. Initialize the
set eagerly and make the field final, removing the null checks in addType
and isType. Behaviour-preserving: an empty set yields the same isType
results the null guards did. The empty-string guard in isType(String) is
kept (it guards input, not the set).

* docs: shorten the resolveGroupReferences interning comment

Trim the inline comment to just what a reader needs at the call site (why
the extra map exists), moving the full rationale here.

How the optimization works
--------------------------
resolveGroupReferences assigns each loaded object to the "TYPE=" group
references it qualifies for. It is a nested scan:

    for each object (getAllObjects)
        for each group reference (typeReferences)
            for each type token in that group's key
                if !object.isType(token) -> object is not in this group

The group key is a FixedStringList such as ["Martial","Melee"]; its
elements are plain strings. The original code called
object.isType(String) on each string token, and isType(String) does, per
call: uppercase the string, tokenize on '.', and Type.getConstant(token)
to intern each piece into the shared Type constant. Type.getConstant is a
CaseInsensitiveMap lookup (hash + case-insensitive equals). Because it sat
in the innermost loop, that interning ran objects x groups x tokens times,
even though the set of group tokens is tiny and fixed for the whole scan.

The fix hoists the interning out of the loop. Before iterating objects we
build internedKeys: for each group key, convert its string tokens to their
Type constants exactly once. getTypeReference (which constructs these keys)
already guarantees every element is a single, plain token -- it rejects
'.', '=', ',', '|' -- so each string maps to exactly one Type.getConstant
with no tokenizing or edge cases. The inner loop then calls the
isType(Type) fast path (a direct containsInList(ListKey.TYPE, type) on
PObject), doing a cheap Type membership test instead of re-interning a
string.

Net effect: Type.getConstant calls drop from O(objects x groups x tokens)
to O(groups x tokens). Correctness is preserved for every element type T:
implementers with special type semantics (notably Equipment, whose isType
merges EqMod-derived types) do not override isType(Type) and inherit
Loadable's default, which delegates back to isType(String).

* test: cover TYPE-group resolution in AbstractReferenceManufacturer

resolveGroupReferences (reached via resolveReferences) had no unit test
pinning its input -> group-membership behaviour, only transitive coverage
that asserted the boolean return, not which objects landed in which group.
The isType(Type) fast-path optimization relies on that behaviour, so add
direct tests: single- and multi-token membership (all tokens must match),
case-insensitive matching, multi-group membership, and the dangling
TYPE= case (matches nothing -> resolveReferences returns false).



To unsubscribe from these emails, change your notification settings at https://github.com/PCGen/pcgen/settings/notifications