Component state rework (WICKET-6774) and further performance improvements by papegaaij · Pull Request #1595 · apache/wicket

papegaaij · 2026-09-11T08:17:28Z

Draft, for discussion. This collects the WICKET-6774 component state rework together with a
handful of further allocation and CPU savings found by profiling a real application, and adds
the benchmark module they were all measured with.

wicket-benchmarks

A new module, in the default reactor so that the benchmarks keep compiling against the current
API. It is never released and has no unit tests; see its README for how to run it.

The component benchmarks used to investigate WICKET-6774 only ever existed as attachments on
the issue, and they no longer compile: WicketTester has moved to its own module, and since
JDK 23 javac no longer runs annotation processors found on the classpath, so JMH silently
produces no BenchmarkList and the run executes nothing. Keeping them in the reactor means
they keep compiling.

It now holds three kinds of tool:

JMH benchmarks - ComponentStateBenchmark, PageRenderBenchmark,
ResourceNameIteratorBenchmark, PageSerializationBenchmark, PageEncryptionBenchmark.
ComponentFootprint - retained heap via JOL and serialized size, per state shape,
against an identical stateless tree so the difference isolates the state itself.
WicketContext - the shared harness.

Component state (WICKET-6774)

A component's state is no longer an Object[] with a packing convention, but a
ComponentState instance. One final class rather than one subclass per combination: every
unpacking call site otherwise dispatches over four implementations of the same six accessors,
which is past the point where HotSpot stops inlining, and a real page interleaves shapes even
where a benchmark feeding one shape at a time would not show it.

Reading state over an array holding every shape at once, 2 forks, ns/op, master -> this branch:

	master	this branch
`readBehaviorsMixedShapes`	31.50	21.35	-32%
`readMetaDataMixedShapes`	25.27	17.66	-30%
`readModelMixedShapes`	30.17	16.54	-45%
`readBehaviors[AJAX_BEHAVIOR]`	11.15	1.97	-82%, 72 -> 24 B/op
`readBehaviorById`	2.82	1.62	-43%

For a component carrying a real AjaxEventBehavior the tree is 39.8% smaller serialized, -72
bytes retained heap and -32 bytes serialized per component - comparable to the -36.2% reported
on the issue in 2020.

Behaviour change: behavior ids are only maintained for stateful behaviors. Ids can change
for other behaviors, also when combined on the same component. This is documented in the guide
commit on this branch.

Resource name iteration

ResourceStreamLocator walks a list of candidate filenames for every property and markup
lookup, once per registered properties loader. Misses are the common case, because a key is
resolved by climbing the component hierarchy, so every class above the one that declares it
contributes a full traversal that finds nothing.

toString() built each candidate from four prepend() calls, and the locale part went through
getLocale() - a Locale.of() cache lookup - even though LocaleResourceNameIterator.next()
had just built the identical suffix and thrown it away. ResourceUtil#rejectPathSeparators
separately ran Locale#toString() on every call.

nl_NL without a style, 3 forks:

	master	this branch
`walkAllCandidates` (miss)	923.8 ns, 1224 B	139.9 ns, 672 B	-85% / -45%
`firstCandidate` (hit)	775.2 ns, 600 B	405.6 ns, 368 B	-48% / -39%

Both changes were verified by enumeration rather than by argument: every candidate name with
its locale, style, variation and extension over 6 paths x 2 styles x 2 variations x 7 locales x
5 extension lists x strict/non-strict (9918 names), and 4918 locales for rejectPathSeparators

byte-identical before and after.

One note for reviewers: toString() now takes the locale segment from next() instead of from
getLocale(). A subclass supplied through the protected newLocaleResourceNameIterator hook
that overrode getLocale() without also overriding next() would see a difference; nothing in
the tree does that.

Page store

Serialization and encryption run on the request thread after the response has been flushed, so
this is throughput and thread occupancy rather than user-visible latency.

JavaSerializer wrote into a java.io.ByteArrayOutputStream with no initial size, which
starts at 32 bytes and grows by copying everything written so far into a buffer of twice the
size. Wicket already has a ByteArrayOutputStream that chains a new buffer instead.

SchemeCrypt prefixes every ciphertext with a one-byte marker naming the scheme, and did so by
allocating a new array one byte longer and copying the whole payload in - and on the way back
copying everything after the marker out again. AbstractAesGcmCryptScheme added a third copy.
ICryptScheme#encrypt/#decrypt now carry the offsets needed to avoid all three; the old
signatures remain as default methods. The marker stays SchemeCrypt's concern, as documented
in the user guide - a scheme still knows nothing about it, only that the first few bytes of
what it returns are not its own.

	master	this branch
serialize, 500 components (37,924 bytes)	256,714 B/op	214,763 B/op	-16%
encrypt, 40kB	18.28 us, 124,496 B	12.58 us, 44,416 B	-31% / -64%
decrypt, 40kB	14.07 us, 84,408 B	11.17 us, 44,360 B	-21% / -47%

Encrypting now allocates 1.11x the payload rather than 3.1x. Serialization time is unchanged;
what remains there is ObjectOutputStream's handle table, which scales with the number of
objects in the page graph rather than with its byte count - which is the thing the component
state rework above reduces.

How the non-6774 hotspots were found

JFR on a large production application under its Selenium suite, keeping only samples whose
stack contains WicketFilter.doFilter, charging JDK frames to the nearest non-JDK caller and
bucketing by package. Inside Wicket request handling: page store 23.2% CPU / 12.4% allocation,
render 16.8% / 15.5%, localizer 6.1% / 15.0%.

Two attempts were measured and dropped rather than included: folding the resource name
iterators into precomputed lists (6x slower on the hit path, because getLocale() costs a
cache lookup per state and the hit path stops early), and a reset()-based restart of the same
iterators (no time win, three new subclass contract hazards).

🤖 Generated with Claude Code

State of a component is no longer stored as an array but using instances of ComponentState. These have a smaller memory footprint in most cases, are more efficient and the code is easier to read (I hope). Note that a small change in behavior is introduced: behavior ids are only maintained for statefull behaviors. Ids can change for other behaviors, also when combined on the same component.

Brings six years of master (1459 commits) into the WICKET-6774 component state rework. Conflict resolutions: * Component.java: master's WICKET-6830 turned `new Behaviors(this).x()` into static `Behaviors.x(this)` calls. All eight conflicting hunks kept this branch's ComponentState equivalents, which supersede that refactor. Master's other changes to the file merged cleanly and are retained. * Behaviors.java: kept deleted. Its responsibilities have moved into ComponentState and nothing references the class any more. * ImmutableBehaviorIdsTest: master moved the test to wicket-core-tests and normalised it to LF line endings, so every line conflicted. Took master's version and reapplied this branch's expected behavior ids (2 and 4), which follow from ids being indices into the behaviors array. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Detaching or removing a behavior runs arbitrary user code, which can replace the component's state object, for example by clearing a meta data entry. ComponentState.detachBehaviors() and removeBehaviors() captured the state before running that code and afterwards wrote the resulting behaviors back into the captured object, silently reverting the change. Master fixed its variant of this in Behaviors.detach() (WICKET-6877). That file is deleted on this branch, so the fix never carried over. The symptom here is different: no behavior is skipped, but a meta data change made from Behavior.detach() is lost. Both methods now return the remaining behaviors rather than a new state, and Component stores them in the state as it is after the callbacks have run. As a side effect removeBehaviors() no longer has to branch on the shape of the state, because setBehaviors() at the call site already handles that. A behavior that adds or removes another behavior from within its own detach() is still overwritten. That limitation also existed with Behaviors. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The component benchmarks used to investigate this issue only ever existed as attachments on WICKET-6774, and they no longer compile: WicketTester has moved to its own module, and since JDK 23 javac no longer runs annotation processors found on the classpath, so JMH silently produces no BenchmarkList and the run executes nothing. Keeping them in the reactor means they keep compiling. Three tools, because the constraints they check are not the same question: * ComponentStateBenchmark - JMH, ns/op and bytes/op for the per-request state accessors. Reads are measured per state shape and again over a component array holding every shape at once. With a single shape the call sites that unpack Component.data are monomorphic and inline, which flatters any implementation dispatching on the shape, while real pages interleave shapes. Mutation is measured as construct-and-detach in one operation, because detach() is not idempotent and so cannot be measured repeatedly against the same instance. * ComponentFootprint - retained heap via JOL and serialized size via Java serialization, per shape, against an identical stateless tree so that the difference isolates the state itself. Neither is a throughput question, and a footprint claim also depends on -XX:+UseCompactObjectHeaders, which can change which layout wins. * PageRenderBenchmark - a full render as an end-to-end regression guard. The seven near-identical methods of the original are replaced by one parameterised over the state shape. The shapes include a behavior that requires a stable id, which is what every link and ajax-enabled component has, and which the original benchmarks never covered even though the largest saving claimed on this issue is there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

ComponentState had four subclasses, one for each combination of model, behaviors and meta data that is worth wrapping. Every unpacking call site therefore dispatched over four implementations of the same six accessors, which is past the point where HotSpot stops inlining a virtual call. Reading state is done all over the framework, and a real page interleaves components of every shape, so those call sites go megamorphic in practice even though a benchmark that feeds one shape at a time will not show it. The specialised classes were not buying anything to offset that. Two and three reference fields both occupy 24 bytes on a 64 bit VM with compressed oops, so one class with three fields is the same size as any of the four it replaces. Measured on a tree of 1000 components, retained heap is identical for every state shape, and serialized form grows by about one byte per component for the two-slot shapes, where the third field is written as a null. A single final class also removes 235 lines, and pack() now holds in one place the rule that used to be spread over twelve setters: a wrapper is only worth allocating while more than one kind of state is present, otherwise the value goes into Component.data directly. Measured with wicket-benchmarks, reading state over an array holding every shape at once, 2 forks, ns/op: readBehaviorsMixedShapes 31.50 -> 21.35 -32% readMetaDataMixedShapes 25.27 -> 17.66 -30% readModelMixedShapes 30.17 -> 21.28 -30% Single-shape reads and allocation per operation are unchanged, as expected: those call sites were already monomorphic, so this only affects dispatch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Reading a model went through the type check on Component.data first and only then consulted the flag, so components without a model paid for the check before being told there was nothing to find. Most components have no model at all: of 548285 components measured on a production application, 61% have none. Testing the flag first skips the check for those entirely. The flag is authoritative, since it is set exactly when a model is stored, which is the same order master uses in getModelImpl(). Measured with wicket-benchmarks over an array holding every state shape at once, 2 forks, ns/op: readModelMixedShapes 21.28 -> 16.54 -22% which brings the total for this path to 30.17 -> 16.54, -45%, against 10.59 for master. The remainder is not explained by dispatch or by check order and would need perfasm to attribute; at roughly 0.6ns per model read it is small next to the gains on the other two accessors. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…s first Every behavior shape so far used AttributeModifier, which never needs a stable id, so none of them created the BehaviorIdList that stores those ids. That is the structure this issue claims its largest saving on, and it was the one case the benchmarks did not cover. AJAX_BEHAVIOR adds a real AjaxEventBehavior, so the numbers are comparable to the -36.2% serialized saving reported on the issue in 2020; measured now it is -39.8% for the tree, -72 bytes of retained heap and -32 bytes serialized per component. Keeping STABLE_ID_BEHAVIOR alongside it is deliberate. The absolute saving is the same for both, because the same structure is removed either way, but the bare behavior carries almost nothing of its own and so reports -86% per component against the real behavior's -68%. Having both makes it obvious that the absolute figure is the one that carries over between cases and the percentage is not. readBehaviorById could not run at all before this. It looked up an id that was never handed out, which the array index of the reworked state resolves happily while master throws InvalidBehaviorIdException, since master only builds its BehaviorIdList when getBehaviorId is called. The setup now assigns ids first, the way rendering does, so the benchmark measures the same work on both. With that fixed, the ajax paths measure (2 forks, ns/op, master -> reworked state): readBehaviorById 2.82 -> 1.62 -43% readBehaviors[AJAX_BEHAVIOR] 11.15 -> 1.97 -82%, 72 -> 24 B/op readMetaData[AJAX_BEHAVIOR] 1.06 -> 0.81 -24% buildAndDetach[AJAX_BEHAVIOR] 51.59 -> 29.36 -43%, 180 -> 96 B/op Meta data reads get cheaper as a side effect: master keeps the id list in the component's own meta data, so any meta data read on a link or ajax component pays to walk past it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

getDefaultModel() behaves differently depending on whether a model is actually there, and only the populated case was being measured. Inferring the empty case by subtracting the all-model array from the mixed array is not valid: the arrays differ in length and in the types seen at the unpacking call site, and doing so pointed an investigation at the wrong cause. Reading each case from its own array instead: readModelAllHaveModel only shapes that carry a model readModelNoneHaveModel only shapes that do not Each read benchmark also gets a baseline twin that walks the same array with the same blackhole and reads a plain field, so the loop and blackhole overhead can be subtracted and what is left is the accessor. That overhead is a large part of these numbers: over eleven components it is roughly 4ns of a 12ns measurement. The README gains the two things that cost the most time to learn: measure a case rather than deriving it from others, and set the fork heap deliberately. Too large is machine dependent, too small is GC noise - the render benchmark reports 274 +- 437 us/op in 1GB and 102 +- 2 us/op in 4GB. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

getDefaultModel() memoises the inherited model lookup: when a component has no model of its own, initModel() walks the parent chain for an IComponentInheritedModel and, on a hit, allocates a wrapper bound to this component, which setModelImpl() then stores so the next call is cheap. The matching invalidation lives in detach(). It also stored the misses, and storing a miss records nothing, because the null leaves FLAG_MODEL_SET clear and the next call walks the hierarchy again anyway. So the write bought nothing while costing two field stores, to data and to flags, on every call for a component without a model. Most components have none: of 548285 components measured on a production application, 61% carry no model. Master pays nothing here only incidentally, its setModelImpl() falling through both branches when the flag is clear. Skipping the call is a no-op by construction. FLAG_MODEL_SET is set exactly when a non-null model was stored, and only setModelImpl() writes the model slot, so getModelImpl() returning null implies the flag is clear; with the flag clear ComponentState.setModel(null, data, false) returns data unchanged down every branch and the setFlag() is already false. Nothing overrides setModelImpl() or getModelImpl(), and initModel() raises FLAG_INHERITABLE_MODEL only on the path that returns non-null. Measured with wicket-benchmarks, reading models over an array of components, harness overhead subtracted, ns per component: master before after model present 1.117 0.871 0.889 model absent 0.316 1.399 0.352 mixed 0.654 1.197 0.340 The absent path was 4.4x master and is now level with it, and reading state over interleaved components is faster than master rather than 83% slower. Note that a miss is still not memoised, on master or here: every getDefaultModel() on a component without a model re-walks its ancestors. Fixing that needs somewhere to record that the lookup already failed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

ResourceStreamLocator walks a list of candidate filenames for every property and markup lookup, once per registered properties loader. That walk dominates i18n lookup cost on a miss, and misses are the common case: a key is resolved by climbing the component hierarchy, so every class above the one that actually declares it contributes a full traversal that finds nothing. The hit and the miss are benchmarked directly rather than derived from one another, because they do different amounts of work, and the read-back of locale, style and variation is included in the hit - it is the only caller of getLocale(), so leaving it out would hide that cost. Parameterised over the locale shapes, which drive how many candidates a traversal produces. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

ResourceNameIterator.toString() built each candidate name from four prepend() calls, each concatenating a char with an Object, so four throwaway strings were allocated on top of the result. The locale part was the expensive one: it went through getLocale(), which calls Locale.of() - a LocaleObjectCache lookup rather than a field read - even though LocaleResourceNameIterator.next() had just built the identical suffix and discarded it. next() now keeps that suffix and toString() reuses it; getLocale() memoises the derived Locale per state for the locator's own read-back; the style and variation parts are rebuilt only when the style iterator advances; and each branch of toString() is a single concatenation, so only the returned string is allocated. ExtensionResourceNameIterator was also building its backing iterator twice. ResourceNameIteratorBenchmark, nl_NL without a style, 3 forks: walkAllCandidates (miss) 923.8 -> 167.8 ns/op 1224 -> 736 B/op firstCandidate (hit) 775.2 -> 421.6 ns/op 600 -> 432 B/op Verified equivalent by enumerating every candidate name and its locale, style, variation and extension across 6 paths x 2 styles x 2 variations x 7 locales x 5 extension lists x strict/non-strict - 9918 names - before and after the change: byte-identical. One behavioural note for reviewers: toString() now takes the locale segment from next() instead of from getLocale(). For the shipped implementation those are provably the same, which is what the enumeration above establishes. A subclass supplied through the protected newLocaleResourceNameIterator hook that overrode getLocale() without also overriding next() would see a difference; nothing in the tree does that. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

ResourceUtil#rejectPathSeparators(Locale) ran Locale#toString() on every call, and that builds a new string each time. It runs once per ResourceNameIterator construction, so three times per property lookup - once per registered properties loader. Without a variant, a script or extensions, Locale#toString() returns nothing but the language and the country joined by '_', so inspecting those two subtags directly is equivalent and allocates nothing. Richer locales keep the general route, and that fallback is load-bearing rather than defensive: Locale#toString() omits a variant that has neither a language nor a country, so Locale.of("", "", "a/b") renders as the empty string and must not be rejected. Checked against the previous implementation over 4918 locales - every combination of 17 subtag values across language, country and variant, plus script and extension shapes and null - with identical results throughout. ResourceNameIteratorBenchmark, nl_NL without a style: walkAllCandidates (miss) 167.8 -> 139.9 ns/op 736 -> 672 B/op firstCandidate (hit) 421.6 -> 405.6 ns/op 432 -> 368 B/op Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

SerializingPageStore turns every page into bytes at the end of a request, and CryptingPageStore encrypts the result when StoreSettings#isEncrypted() is on. Neither path had a benchmark. Both are parameterised by size rather than run on one shape, because that is what they scale with. Serialization tracks the number of objects in the graph - ObjectOutputStream keeps a handle table per stream and grows it as it walks, so a page of many small components costs more than its byte count suggests - while encryption tracks the payload, so anything it does per byte is invisible in a benchmark over short strings. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

JavaSerializer wrote pages into a java.io.ByteArrayOutputStream with no initial size. That starts at 32 bytes and grows by copying everything written so far into a buffer of twice the size, so a page of any substance is copied a dozen times over on its way out, and every one of those buffers is discarded again immediately. Wicket already has a ByteArrayOutputStream that chains a new buffer instead of copying, which is what this switches to, starting at a size no page worth storing comes in under. PageSerializationBenchmark, per page: 500 components (37,924 bytes) 256,714 -> 214,763 B/op 50 components ( 5,173 bytes) 33,592 -> 33,000 B/op The saving grows with the page, as the copying it removes did: at 500 components it is larger than the serialized page itself. Time is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

SchemeCrypt prefixes every ciphertext with a one-byte marker naming the scheme that produced it. It did so by allocating a new array one byte longer and copying the whole ciphertext into it, and on the way back by copying everything after the marker out again - two full copies of a page-sized payload for the sake of one byte. AbstractAesGcmCryptScheme added a third, taking the ciphertext as an array of its own before copying it in behind the nonce. ICryptScheme's encrypt and decrypt now carry the offsets needed to avoid all three. encrypt leaves prefixLength bytes free at the front of the result, so SchemeCrypt writes its marker in place; decrypt takes an offset and a length, so SchemeCrypt hands over the buffer it already has and simply skips the marker. The old signatures remain as default methods delegating with 0 and the full range. The marker stays SchemeCrypt's concern, as documented in the user guide - a scheme still knows nothing about it, only that the first few bytes of what it returns are not its own. PageEncryptionBenchmark, 40kB payload: encrypt 18.28 -> 12.58 us/op 124,496 -> 44,416 B/op decrypt 14.07 -> 11.17 us/op 84,408 -> 44,360 B/op Encrypting a payload now allocates 1.11 times its size rather than 3.1 times: one array and no copies, where there were three arrays and two copies. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

codecov-commenter · 2026-09-11T08:26:42Z

Codecov Report

❌ Patch coverage is 84.19689% with 61 lines in your changes missing coverage. Please review.
✅ Project coverage is 61.90%. Comparing base (9a598e5) to head (e8acb1c).
⚠️ Report is 34 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #1595      +/-   ##
============================================
+ Coverage     61.84%   61.90%   +0.06%     
- Complexity    11183    11234      +51     
============================================
  Files          1246     1247       +1     
  Lines         48231    48338     +107     
  Branches       6759     6783      +24     
============================================
+ Hits          29830    29926      +96     
- Misses        15700    15705       +5     
- Partials       2701     2707       +6

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

The skip was added on the assumption that a new module has no previous release to compare against. It does: japicmp's oldVersion resolves ${project.artifactId} at 11.0.0-SNAPSHOT, which the workspace reader supplies from the module's own build, so it compares target/wicket-benchmarks-11.0.0-SNAPSHOT.jar against that same file and reports no changes. Nothing needs to be installed for that to hold. The note on the dependency block goes with it; the order speaks for itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

WicketObjects gained an import of java.nio.file.Files in the first component state commit, and that import was the whole of what the commit changed in that file. With it gone the file is identical to master again, so the branch no longer touches it at all. PageSerializationBenchmark still imported Component, ListItem and ListView from when it built its page with a ListView, before a flat loop replaced it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Component#add used to go through Behaviors#add, which rejected a null array and a null element with Args#notNull before touching anything. Folding that method into Component dropped both checks. add(null) became a NullPointerException from inside ComponentState, and add(behavior, null) threw only after the first behavior had already been stored and bound, leaving the null holding a slot of its own: nothing iterates it, but it still shifts every id handed out after it. The checks run in a loop ahead of the one that binds, so that nothing is stored when a later element turns out to be null. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Cipher#getOutputSize only promises an upper bound on what the next doFinal will write. encrypt sized the result from it and ignored what doFinal returned. AES/GCM in SunJCE and AES/GCM-SIV in Bouncy Castle both fill it exactly, so neither shipped scheme was ever affected, but getCipher is a protected hook: a provider that reserves more than it writes would leave zeroes at the end of the ciphertext and fail authentication on the way back, with nothing to point at the cause. The written length now decides, so the buffer is returned untouched when it was filled exactly - the only case the shipped schemes take - and copied down only when it was not. decrypt takes an offset and a length from its caller and passed them straight to Arrays#copyOfRange, so a range outside the array surfaced as an array index error from inside the scheme. It is checked up front instead, which leaves a short but well-formed range returning null as documented. The javadoc on the three ICryptScheme methods and on SchemeCrypt's marker helper was left doubled when the offsets were added, so the block that described the original contract was orphaned and the one that survived on encryptDeterministic no longer mentioned why its output has to be stable. Each is now a single block. The helper is renamed to writeMarker: it no longer prefixes anything, it writes into the slot the scheme was asked to leave free. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

An id is a position in the component's list of behaviors, so ids are not dense, do not necessarily start at zero, and which one a behavior gets depends on what else was added before an id was first asked for. None of that was written down. The contract also promised that the bookkeeping behind these ids costs memory. There is no BehaviorIdList any more; what it costs now is that the list stops being compacted once an id has been handed out, so the gaps left by removed behaviors are kept. getBehaviorById is documented as returning null when there is no such behavior, which it has never done - it throws InvalidBehaviorIdException. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

INITIAL_BUFFER_SIZE was declared between applicationKey's javadoc and the field itself, which left the field undocumented. Its own comment claimed that no page worth storing serializes into less than the buffer, which is not true of a small one; it now just says what the size is for. wicket-coverage lists every module deliberately left out of the aggregate report together with the reason it is left out. wicket-benchmarks was missing from that list. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

reiern70

I'm approving. But this is very difficullt to review. I just made some comments that might make sense or not. It would be nice to port this to 10.x and also host a 10.x. SNAPSHOT somewhere so that we can test this with other applications (e.g. our application). Thanks for all your work!

reiern70 · 2026-09-11T16:23:10Z

+			return c;
+		}
+
+		void populate(Component c)


Maybe:

Make create interface IShape with a method populate(Component c)

Then you make Shape implement this interface and a given enum contant implements populate

Then you don't need the booleans and each populate clearly show what it is populating?

You are right about the symptom: AJAX_BEHAVIOR(false, false, false, true, true) was unreadable, and two of the three constructors existed only to fill in the defaults the shorter constants did not name.

I went for a variation on your suggestion rather than constant-specific populate bodies. Eight of the eleven constants are exactly the combinations of three independent ingredients, so a populate per constant would repeat the same three snippets across those combinations, and hasModel() - which the ModelShapes and NoModelShapes states use to split the constants - would need an override per constant as well.

So the ingredients became a Trait enum, collected into an EnumSet by a varargs constructor:

MODEL_BEHAVIOR(Trait.MODEL, Trait.BEHAVIOR), STABLE_ID_BEHAVIOR(Trait.STABLE_ID), AJAX_BEHAVIOR(Trait.STABLE_ID, Trait.AJAX);

One constructor, no positional booleans, each constant naming what it carries, populate still in one place and hasModel() reduced to traits.contains(Trait.MODEL). The javadoc that hung on the stable-id and ajax constants moved onto the traits it actually describes. Constant order is unchanged, so the ordinals and the @Param strings the benchmarks are run with still line up. 3ef485a.

reiern70 · 2026-09-11T16:27:03Z

 	}

+	@Test
+	void nullBehaviorIsRejectedBeforeAnythingIsStored() {


Added in f2c78b1. It now says what the test protects rather than only what it asserts: add validates the whole argument list before it stores any of it, because a behavior's id is its position in the list, so one that was already stored when a later null was rejected would keep the position it took and shift the id of everything added after it.

reiern70 · 2026-09-11T16:30:06Z

 		final Component component = getComponent();

 		component.setOutputMarkupId(true);
-


Why is this no longer needed?

Because an id no longer has to be handed out in order to exist.

On master an id is a position in a BehaviorIdList kept in the component's meta data, allocated in the order getBehaviorId is first called. That order is whatever rendering happens to do, which is not reproducible for a stateless page: the page is thrown away and rebuilt on the next request, while the id is already baked into the callback url in the markup. Calling getBehaviorId from onBind pinned the id to add order instead, which is reproducible. That is what the block was for.

On this branch an id is the behavior's index in the component's behavior array, so it is add order by construction and there is nothing left to force.

Doing it eagerly would now cost something, too. getBehaviorId compacts the behavior array one last time and then sets FLAG_BEHAVIOR_IDS_FIXED, which stops it from ever being compacted again so that the ids it hands out stay put. Calling it at bind time would set that flag on every component carrying an ajax behavior, so it would keep the gaps left by removed behaviors for the rest of its life - exactly the footprint this PR is trying to get rid of. The stability guarantee itself is unchanged; it just starts at first use rather than at bind. I wrote the contract down on IRequestableComponent#getBehaviorId in 7323aca.

reiern70 · 2026-09-11T16:39:38Z

+		Iterator<String> extensionIterator = extensions == null ? null : extensions.iterator();
+		if (extensionIterator == null || !extensionIterator.hasNext())
 		{
 			this.iterator = NULL_ITERABLE.iterator();


Is this thread safe? If not... is this not important?

It is, and this change does not move that either way.

ExtensionResourceNameIterator instances are per lookup - ResourceNameIterator constructs one per locate() - so they are never shared between threads. The only shared state is NULL_ITERABLE, which predates this branch: a one-element Arrays.asList that is never mutated, and whose iterator() hands out a fresh ArrayItr with its own cursor on every call. ArrayItr does not implement remove(), so ExtensionResourceNameIterator.remove() throws UnsupportedOperationException on that path rather than touching the shared list.

What the change does is ask extensions for an iterator once instead of twice. hasNext() does not advance, so reusing the iterator the check was made on is equivalent to throwing it away and asking for a second one.

reiern70 · 2026-09-11T16:59:46Z

+ * size of this state as small as possible, the following cases are identified:
+ * <ul>
+ * <li>No state at all: {@code data} is {@code null}
+ * <li>Only a model: {@code data} contains the model


Not belonging to this PR... but many times we have components with several models and then we need to remmeber to manually detach them... maybe wicket should support registering other models and detaching them?

Agreed that it is a real gap. IModel has detach(), but nothing calls it unless the model is the component's default model or is wrapped by something that passes the call on, so every extra model a component keeps is a detach you have to remember to write yourself.

It is new API rather than a side effect of this rework though, so I would rather not fold it into this PR. Could you open a GitHub issue for it? JIRA was shut down yesterday, so that is where it would live now.

As you may have noticed, this Claude did the replying on your comments. Maybe I should have reviewed them first 😂

papegaaij · 2026-09-11T19:51:18Z

I'm approving. But this is very difficullt to review. I just made some comments that might make sense or not. It would be nice to port this to 10.x and also host a 10.x. SNAPSHOT somewhere so that we can test this with other applications (e.g. our application). Thanks for all your work!

It should not be that hard to backport most of this to 10.x, but I'd rather not ship it in a 10.x release. I'm pretty confident that the code is functionally equivalent, but there may be very subtle changes in behavior. One such change is that behaviors can get different ids. This should normally not make any difference, as long as the id is stable over requests, but someone might depend on these numbers. I can create a branch on 10.x for you to test your application against.

@PARAM

The constants are combinations of independent ingredients, and passing those positionally meant AJAX_BEHAVIOR(false, false, false, true, true) at the call site, plus three constructors whose only job was to supply the defaults the shorter constants did not name. A Trait enum collected into an EnumSet names each ingredient where it is used, needs one constructor, and leaves populate() as the single place that reads them. Constant order is unchanged, so ordinals and the @PARAM strings the benchmarks are run with still line up. The javadoc that hung on the stable-id and ajax constants moved onto the traits it actually describes, and the enum no longer claims eight shapes when there are eleven. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The test asserts that a rejected add leaves nothing behind, which only matters because a behavior's id is its position in the list: one stored ahead of the null would keep the slot it took and shift the id of everything added after it. The inline comment said that about the assertion, the method itself said nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

reiern70 · 2026-09-11T19:58:49Z

I'm approving. But this is very difficullt to review. I just made some comments that might make sense or not. It would be nice to port this to 10.x and also host a 10.x. SNAPSHOT somewhere so that we can test this with other applications (e.g. our application). Thanks for all your work!

It should not be that hard to backport most of this to 10.x, but I'd rather not ship it in a 10.x release. I'm pretty confident that the code is functionally equivalent, but there may be very subtle changes in behavior. One such change is that behaviors can get different ids. This should normally not make any difference, as long as the id is stable over requests, but someone might depend on these numbers. I can create a branch on 10.x for you to test your application against.

yes please... It would also be nice if we could publish those temporary fixes somewhere with /branchname/m2 repository, this way I can point our gladle to that repository and build as we nomally build (similar to the staging repositories whne releases are voted).

…ments

papegaaij and others added 24 commits April 30, 2020 13:40

WICKET-6774: do not recreate state when not needed

d091fe0

WICKET-6774: inline state unpacking to improve performance

a354416

WICKET-6774: minor performance improvements, fixed header

091d0e0

WICKET-6774: mutable state

8ba675c

WICKET-6774: add documentation

2628cb6

WICKET-6774: keep behavior ids stable during request

343b0d2

Merge branch 'master' into wicket-6774

e27b56e

WICKET-6774: Use flag to fixate behavior ids

6174a1e

Merge remote-tracking branch 'origin/master' into wicket-6774

5f67689

papegaaij and others added 2 commits September 11, 2026 09:32

reiern70 reviewed Sep 11, 2026

View reviewed changes

Comment thread wicket-benchmarks/src/main/java/org/apache/wicket/benchmarks/PageSerializationBenchmark.java Outdated

papegaaij and others added 2 commits September 11, 2026 13:50

papegaaij and others added 2 commits September 11, 2026 13:51

papegaaij marked this pull request as ready for review September 11, 2026 13:54

reiern70 approved these changes Sep 11, 2026

View reviewed changes

papegaaij and others added 2 commits September 11, 2026 21:51

Merge remote-tracking branch 'origin/master' into performance-improve…

e8acb1c

…ments

papegaaij merged commit ff0289a into master Sep 14, 2026
3 checks passed

papegaaij deleted the performance-improvements branch September 14, 2026 19:45

		final Component component = getComponent();

		component.setOutputMarkupId(true);

Conversation

papegaaij commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

wicket-benchmarks

Component state (WICKET-6774)

Resource name iteration

Page store

How the non-6774 hotspots were found

Uh oh!

codecov-commenter commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

reiern70 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

papegaaij commented Sep 11, 2026

Uh oh!

reiern70 commented Sep 11, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

papegaaij commented Sep 11, 2026 •

edited

Loading

codecov-commenter commented Sep 11, 2026 •

edited

Loading