[
https://issues.apache.org/jira/browse/IGNITE-29109?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18123845#comment-18123845
]
Nikolay Izhikov commented on IGNITE-29109:
------------------------------------------
Allocation measurements added to the reproducer. The measuring loop now reads
{{com.sun.management.ThreadMXBean#getCurrentThreadAllocatedBytes}} before and
after each round, so the byte column is exact per operation, not sampled. All
three versions on the same machine, JDK 17.0.16, 12 CPUs, one embedded server
node, single thread, median of 5 rounds of 20 000 ops after warm-up. "fixed" is
master + the patch from this ticket.
h4. toBinary
||Field value||2.17.0 ns||2.17.0 B||2.18.0 ns||2.18.0 B||fixed ns||fixed B||
|empty HashMap|490|488|452|488|359|480|
|empty ArrayList|277|472|308|472|243|472|
|empty TreeMap|495|624|*6704*|*11641*|281|516|
|empty TreeSet|527|584|*6167*|*9088*|358|528|
|TreeMap, 12 entries|1107|2456|*7106*|*12152*|966|728|
h4. cache.put
||Field value||2.17.0 ns||2.17.0 B||2.18.0 ns||2.18.0 B||fixed ns||fixed B||
|empty HashMap|2462|2216|2886|2296|1209|2224|
|empty ArrayList|863|2160|913|2216|876|2176|
|empty TreeMap|1202|2323|*9090*|*18936*|992|2256|
|empty TreeSet|991|2328|*8340*|*16328*|988|2288|
|TreeMap, 12 entries|1806|4432|*9911*|*19672*|1696|2720|
h4. cache.get
||Field value||2.17.0 ns||2.17.0 B||2.18.0 ns||2.18.0 B||fixed ns||fixed B||
|empty HashMap|1724|1264|1541|1240|995|1240|
|empty ArrayList|684|1160|793|1216|444|1216|
|empty TreeMap|713|1360|*4594*|*8664*|532|1416|
|empty TreeSet|572|1408|533|1464|533|1464|
|TreeMap, 12 entries|1102|4760|*5161*|*12072*|1110|4832|
Notes:
* On 2.18.0 marshalling an empty {{TreeMap}} allocates ~11 KB per call against
~500 B on 2.17.0 and with the fix. The extra bytes are the throwaway
{{BinaryClassDescriptor}} built by {{makeRegistered()}} on every write,
including the {{NoSuchMethodException}} instances with stack traces from its
reflective look-ups. {{cache.put}} of a {{TreeMap}} pays it twice (for
{{TreeMap}} and its write replacement {{BinaryTreeMap}}), hence ~17 KB extra;
{{cache.get}} pays once on the read side.
* {{TreeSet}} {{cache.get}} is unaffected in all versions, since deserializing
a {{TreeSet}} does not go through the {{TreeMap}} descriptor path.
* With the fix {{TreeMap}} and {{TreeSet}} allocate within a few dozen bytes of
{{HashMap}} and {{ArrayList}} on every operation, and match or beat 2.17.0.
* The 12-entry {{TreeMap}} allocates less with the fix than on 2.17.0 (728 B vs
2456 B on toBinary). This is not from this patch: the 2.18.0 run with
descriptors patched to registered state shows the same saving, so it comes from
other changes to the {{TreeMap}} write path in 2.18 and was masked by the
regression.
* The {{HashMap}} row is measured first in each run and absorbs JIT warm-up, so
its timing differences across versions are noise; its byte counts are stable.
> TreeMap/TreeSet class descriptor is rebuilt on every binary write since
> 2.18.0 (regression of IGNITE-26061)
> -----------------------------------------------------------------------------------------------------------
>
> Key: IGNITE-29109
> URL: https://issues.apache.org/jira/browse/IGNITE-29109
> Project: Ignite
> Issue Type: Bug
> Reporter: Nikolay Izhikov
> Assignee: Nikolay Izhikov
> Priority: Major
> Time Spent: 10m
> Remaining Estimate: 0h
>
> h3. Summary
> Since 2.18.0 every binary write of a {{java.util.TreeMap}} or
> {{java.util.TreeSet}} rebuilds its {{BinaryClassDescriptor}} by reflection.
> An empty {{TreeMap}} costs ~25x more to marshal than in 2.17.0 (~6 µs vs
> ~0.26 µs); {{cache.put}} is ~10x slower and {{cache.get}} of a {{TreeMap}}
> value ~9x slower. {{HashMap}} and {{ArrayList}} are not affected. The compute
> closure wrappers {{GridClosureProcessor.C1/C1MLA/C2/C2MLA/C4/C4MLA}} take the
> same slow path. Reported on the dev list by a user; reproducer attached.
> h3. Measurements
> Reporter's reproducer (attached), one embedded server node, single thread,
> median of 5 rounds of 20 000 ops after warm-up, JDK 17, 12 CPUs, ns/op:
> ||Field value||2.17.0 toBinary||2.18.0 toBinary||2.17.0 put||2.18.0
> put||2.17.0 get||2.18.0 get||
> |empty HashMap|395|521|1602|2669|1142|1673|
> |empty ArrayList|289|292|865|957|472|516|
> |empty TreeMap|261|*6221*|985|*9416*|532|*4728*|
> |empty TreeSet|364|*5722*|990|*8563*|587|556|
> |TreeMap, 12 entries|1098|*6956*|1873|*10208*|1139|*5335*|
> h3. Root cause
> Introduced by IGNITE-26061 (commit 06a557b2e4, PR #12230).
> # {{BinaryContext}} constructor now calls
> {{registerBinarilizableSystemClass}} for {{BinaryTreeMap}}, {{TreeMap}} and
> {{TreeSet}}; {{GridClosureProcessor.onBinaryContextCreated}} does the same
> for the six closure wrappers.
> # {{registerBinarilizableSystemClass}} puts {{systemClassDescriptor(...)}}
> into {{descByCls}}. That descriptor is created with {{registered = false}}.
> # On every write {{registerClass}} sees {{!desc.registered()}} and calls
> {{registerDescriptor}}, which builds a registered copy with
> {{desc.makeRegistered()}} (full {{BinaryClassDescriptor}} constructor with
> reflective look-ups) and offers it with {{descByCls.putIfAbsent(...)}}.
> # The unregistered entry is already present, so {{putIfAbsent}} returns it
> and the registered copy is discarded. The next write repeats the work.
> Before IGNITE-26061 these classes were listed by name in
> {{BINARYLIZABLE_SYS_CLSS}} and nothing was in {{descByCls}} before the first
> write, so the first {{putIfAbsent}} cached the registered descriptor.
> h3. Fix
> Cache the descriptor in the registered state:
> {code:java}
> public void registerBinarilizableSystemClass(Class<?> cls) {
> String clsName = cls.getName();
> descByCls.put(cls, systemClassDescriptor(cls, clsName, new
> BinaryReflectiveSerializer()).makeRegistered());
> }
> {code}
> With the fix the reproducer shows empty {{TreeMap}} at 453 ns toBinary / 1119
> ns put / 580 ns get, i.e. back at 2.17.0 level. A regression test is added to
> {{BinaryContextPredefinedTypesTest}} asserting that the cached descriptors of
> these system classes are registered and that {{registerClass}} returns the
> cached instance.
> h3. Reproducer
> {{treemap-descriptor-repro.zip}} (attached): {{mvn -q compile exec:exec}}
> runs against 2.18.0, {{-Dignite.version=2.17.0}} for comparison. See its
> README.md.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
