[jira] [Commented] (IGNITE-29109) TreeMap/TreeSet class descriptor is rebuilt on every binary write since 2.18.0 (regression of IGNITE-26061)

["Nikolay Izhikov (Jira)" <ji...@apache.org>]

    [ 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)