alex-plekhanov commented on code in PR #12980: URL: https://github.com/apache/ignite/pull/12980#discussion_r3081407839
########## modules/benchmarks/src/test/java/org/apache/ignite/internal/benchmarks/jmh/communication/JmhCacheMetricsSerializationBenchmarkTest.java: ########## @@ -0,0 +1,217 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.internal.benchmarks.jmh.communication; + +import org.apache.commons.lang3.reflect.FieldUtils; +import org.apache.ignite.internal.processors.cluster.CacheMetricsMessage; +import org.junit.Before; +import org.junit.Test; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +/** + * Correctness test for {@link JmhCacheMetricsSerializationBenchmark}. + * + * <p>Verifies that both the new (MessageSerializer) and legacy (inline writeTo/readFrom) + * approaches produce identical round-trip results, and that all benchmark methods + * complete without errors. + */ +public class JmhCacheMetricsSerializationBenchmarkTest { + /** */ + private JmhCacheMetricsSerializationBenchmark benchmark; + + /** */ + @Before + public void setUp() throws Exception { + benchmark = new JmhCacheMetricsSerializationBenchmark(); + benchmark.setup(); + } + + /** New writeTo must complete in a single call. */ + @Test + public void testNewWriteToCompletes() { + assertTrue("newWriteTo must return true", benchmark.newWriteTo()); + } + + /** New readFrom must complete in a single call. */ + @Test + public void testNewReadFromCompletes() { + assertTrue("newReadFrom must return true", benchmark.newReadFrom()); + } + + /** Legacy writeTo must complete in a single call. */ + @Test + public void testLegacyWriteToCompletes() { + assertTrue("legacyWriteTo must return true", benchmark.legacyWriteTo()); + } + + /** Legacy readFrom must complete in a single call. */ + @Test + public void testLegacyReadFromCompletes() { + assertTrue("legacyReadFrom must return true", benchmark.legacyReadFrom()); + } + + /** Round-trip for new approach preserves all fields. */ + @Test + public void testNewRoundTripPreservesFields() throws Exception { + assertTrue(benchmark.newWriteTo()); + assertTrue(benchmark.newReadFrom()); + + var src = (CacheMetricsMessage)FieldUtils.readField(benchmark, "newMsg", true); + var dst = (CacheMetricsMessage)FieldUtils.readField(benchmark, "newReadTarget", true); + + assertCacheMetricsEqual(src, dst); + } + + /** Round-trip for legacy approach preserves all fields. */ + @Test + public void testLegacyRoundTripPreservesFields() throws Exception { + assertTrue(benchmark.legacyWriteTo()); + assertTrue(benchmark.legacyReadFrom()); + + var src = (LegacyCacheMetricsMessage)FieldUtils.readField(benchmark, "legacyMsg", true); + var dst = (LegacyCacheMetricsMessage)FieldUtils.readField(benchmark, "legacyReadTarget", true); + + assertLegacyCacheMetricsEqual(src, dst); + } + + // ----- helpers ----- + + /** Asserts all 87 fields of {@link CacheMetricsMessage} are equal. */ + private static void assertCacheMetricsEqual(CacheMetricsMessage src, CacheMetricsMessage dst) { + assertEquals("cacheGets", src.cacheGets, dst.cacheGets); + assertEquals("cachePuts", src.cachePuts, dst.cachePuts); + assertEquals("entryProcessorPuts", src.entryProcessorPuts, dst.entryProcessorPuts); + assertEquals("entryProcessorReadOnlyInvocations", src.entryProcessorReadOnlyInvocations, dst.entryProcessorReadOnlyInvocations); + assertEquals("entryProcessorAverageInvocationTime", + src.entryProcessorAverageInvocationTime, dst.entryProcessorAverageInvocationTime, 0.0f); + assertEquals("entryProcessorInvocations", src.entryProcessorInvocations, dst.entryProcessorInvocations); + assertEquals("entryProcessorRemovals", src.entryProcessorRemovals, dst.entryProcessorRemovals); + assertEquals("entryProcessorMisses", src.entryProcessorMisses, dst.entryProcessorMisses); + assertEquals("entryProcessorHits", src.entryProcessorHits, dst.entryProcessorHits); + assertEquals("entryProcessorMissPercentage", + src.entryProcessorMissPercentage, dst.entryProcessorMissPercentage, 0.0f); + assertEquals("entryProcessorHitPercentage", + src.entryProcessorHitPercentage, dst.entryProcessorHitPercentage, 0.0f); + assertEquals("entryProcessorMaxInvocationTime", src.entryProcessorMaxInvocationTime, dst.entryProcessorMaxInvocationTime, 0.0f); + assertEquals("entryProcessorMinInvocationTime", src.entryProcessorMinInvocationTime, dst.entryProcessorMinInvocationTime, 0.0f); + assertEquals("cacheHits", src.cacheHits, dst.cacheHits); + assertEquals("cacheMisses", src.cacheMisses, dst.cacheMisses); + assertEquals("cacheTxCommits", src.cacheTxCommits, dst.cacheTxCommits); + assertEquals("cacheTxRollbacks", src.cacheTxRollbacks, dst.cacheTxRollbacks); + assertEquals("cacheEvictions", src.cacheEvictions, dst.cacheEvictions); + assertEquals("cacheRemovals", src.cacheRemovals, dst.cacheRemovals); + assertEquals("averagePutTime", src.averagePutTime, dst.averagePutTime, 0.0f); + assertEquals("averageGetTime", src.averageGetTime, dst.averageGetTime, 0.0f); + assertEquals("averageRemoveTime", src.averageRemoveTime, dst.averageRemoveTime, 0.0f); + assertEquals("averageTxCommitTime", src.averageTxCommitTime, dst.averageTxCommitTime, 0.0f); + assertEquals("averageTxRollbackTime", src.averageTxRollbackTime, dst.averageTxRollbackTime, 0.0f); + assertEquals("cacheName", src.cacheName, dst.cacheName); + assertEquals("offHeapGets", src.offHeapGets, dst.offHeapGets); + assertEquals("offHeapPuts", src.offHeapPuts, dst.offHeapPuts); + assertEquals("offHeapRemoves", src.offHeapRemoves, dst.offHeapRemoves); + assertEquals("offHeapEvicts", src.offHeapEvicts, dst.offHeapEvicts); + assertEquals("offHeapHits", src.offHeapHits, dst.offHeapHits); + assertEquals("offHeapMisses", src.offHeapMisses, dst.offHeapMisses); + assertEquals("offHeapEntriesCnt", src.offHeapEntriesCnt, dst.offHeapEntriesCnt); + assertEquals("heapEntriesCnt", src.heapEntriesCnt, dst.heapEntriesCnt); + assertEquals("offHeapPrimaryEntriesCnt", src.offHeapPrimaryEntriesCnt, dst.offHeapPrimaryEntriesCnt); + assertEquals("offHeapBackupEntriesCnt", src.offHeapBackupEntriesCnt, dst.offHeapBackupEntriesCnt); + assertEquals("offHeapAllocatedSize", src.offHeapAllocatedSize, dst.offHeapAllocatedSize); + assertEquals("size", src.size, dst.size); + assertEquals("cacheSize", src.cacheSize, dst.cacheSize); + assertEquals("keySize", src.keySize, dst.keySize); + assertEquals("empty", src.empty, dst.empty); + assertEquals("dhtEvictQueueCurrSize", src.dhtEvictQueueCurrSize, dst.dhtEvictQueueCurrSize); + assertEquals("txThreadMapSize", src.txThreadMapSize, dst.txThreadMapSize); + assertEquals("txXidMapSize", src.txXidMapSize, dst.txXidMapSize); + assertEquals("txCommitQueueSize", src.txCommitQueueSize, dst.txCommitQueueSize); + assertEquals("txPrepareQueueSize", src.txPrepareQueueSize, dst.txPrepareQueueSize); + assertEquals("txStartVerCountsSize", src.txStartVerCountsSize, dst.txStartVerCountsSize); + assertEquals("txCommittedVersionsSize", src.txCommittedVersionsSize, dst.txCommittedVersionsSize); + assertEquals("txRolledbackVersionsSize", src.txRolledbackVersionsSize, dst.txRolledbackVersionsSize); + assertEquals("txDhtThreadMapSize", src.txDhtThreadMapSize, dst.txDhtThreadMapSize); + assertEquals("txDhtXidMapSize", src.txDhtXidMapSize, dst.txDhtXidMapSize); + assertEquals("txDhtCommitQueueSize", src.txDhtCommitQueueSize, dst.txDhtCommitQueueSize); + assertEquals("txDhtPrepareQueueSize", src.txDhtPrepareQueueSize, dst.txDhtPrepareQueueSize); + assertEquals("txDhtStartVerCountsSize", src.txDhtStartVerCountsSize, dst.txDhtStartVerCountsSize); + assertEquals("txDhtCommittedVersionsSize", src.txDhtCommittedVersionsSize, dst.txDhtCommittedVersionsSize); + assertEquals("txDhtRolledbackVersionsSize", src.txDhtRolledbackVersionsSize, dst.txDhtRolledbackVersionsSize); + assertEquals("writeBehindEnabled", src.writeBehindEnabled, dst.writeBehindEnabled); + assertEquals("writeBehindFlushSize", src.writeBehindFlushSize, dst.writeBehindFlushSize); + assertEquals("writeBehindFlushThreadCnt", src.writeBehindFlushThreadCnt, dst.writeBehindFlushThreadCnt); + assertEquals("writeBehindFlushFreq", src.writeBehindFlushFreq, dst.writeBehindFlushFreq); + assertEquals("writeBehindStoreBatchSize", src.writeBehindStoreBatchSize, dst.writeBehindStoreBatchSize); + assertEquals("writeBehindTotalCriticalOverflowCnt", + src.writeBehindTotalCriticalOverflowCnt, dst.writeBehindTotalCriticalOverflowCnt); + assertEquals("writeBehindCriticalOverflowCnt", src.writeBehindCriticalOverflowCnt, dst.writeBehindCriticalOverflowCnt); + assertEquals("writeBehindErrorRetryCnt", src.writeBehindErrorRetryCnt, dst.writeBehindErrorRetryCnt); + assertEquals("writeBehindBufSize", src.writeBehindBufSize, dst.writeBehindBufSize); + assertEquals("totalPartitionsCnt", src.totalPartitionsCnt, dst.totalPartitionsCnt); + assertEquals("rebalancingPartitionsCnt", src.rebalancingPartitionsCnt, dst.rebalancingPartitionsCnt); + assertEquals("rebalancedKeys", src.rebalancedKeys, dst.rebalancedKeys); + assertEquals("estimatedRebalancingKeys", src.estimatedRebalancingKeys, dst.estimatedRebalancingKeys); + assertEquals("keysToRebalanceLeft", src.keysToRebalanceLeft, dst.keysToRebalanceLeft); + assertEquals("rebalancingKeysRate", src.rebalancingKeysRate, dst.rebalancingKeysRate); + assertEquals("rebalancingBytesRate", src.rebalancingBytesRate, dst.rebalancingBytesRate); + assertEquals("rebalanceStartTime", src.rebalanceStartTime, dst.rebalanceStartTime); + assertEquals("rebalanceFinishTime", src.rebalanceFinishTime, dst.rebalanceFinishTime); + assertEquals("rebalanceClearingPartitionsLeft", src.rebalanceClearingPartitionsLeft, dst.rebalanceClearingPartitionsLeft); + assertEquals("keyType", src.keyType, dst.keyType); + assertEquals("valType", src.valType, dst.valType); + assertEquals("storeByVal", src.storeByVal, dst.storeByVal); + assertEquals("statisticsEnabled", src.statisticsEnabled, dst.statisticsEnabled); + assertEquals("managementEnabled", src.managementEnabled, dst.managementEnabled); + assertEquals("readThrough", src.readThrough, dst.readThrough); + assertEquals("writeThrough", src.writeThrough, dst.writeThrough); + assertEquals("validForReading", src.validForReading, dst.validForReading); + assertEquals("validForWriting", src.validForWriting, dst.validForWriting); + assertEquals("txKeyCollisions", src.txKeyCollisions, dst.txKeyCollisions); + assertEquals("idxRebuildInProgress", src.idxRebuildInProgress, dst.idxRebuildInProgress); + assertEquals("idxRebuildKeyProcessed", src.idxRebuildKeyProcessed, dst.idxRebuildKeyProcessed); + assertEquals("idxBuildPartitionsLeftCount", src.idxBuildPartitionsLeftCount, dst.idxBuildPartitionsLeftCount); + } + + /** Asserts all 87 fields of {@link LegacyCacheMetricsMessage} are equal. */ Review Comment: All 87 fields? :) ########## modules/benchmarks/src/test/java/org/apache/ignite/internal/benchmarks/jmh/communication/JmhCacheMetricsSerializationBenchmarkTest.java: ########## @@ -0,0 +1,217 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.internal.benchmarks.jmh.communication; + +import org.apache.commons.lang3.reflect.FieldUtils; +import org.apache.ignite.internal.processors.cluster.CacheMetricsMessage; +import org.junit.Before; +import org.junit.Test; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +/** + * Correctness test for {@link JmhCacheMetricsSerializationBenchmark}. + * + * <p>Verifies that both the new (MessageSerializer) and legacy (inline writeTo/readFrom) + * approaches produce identical round-trip results, and that all benchmark methods + * complete without errors. + */ +public class JmhCacheMetricsSerializationBenchmarkTest { + /** */ + private JmhCacheMetricsSerializationBenchmark benchmark; + + /** */ + @Before + public void setUp() throws Exception { + benchmark = new JmhCacheMetricsSerializationBenchmark(); + benchmark.setup(); + } + + /** New writeTo must complete in a single call. */ + @Test + public void testNewWriteToCompletes() { + assertTrue("newWriteTo must return true", benchmark.newWriteTo()); + } + + /** New readFrom must complete in a single call. */ + @Test + public void testNewReadFromCompletes() { + assertTrue("newReadFrom must return true", benchmark.newReadFrom()); + } + + /** Legacy writeTo must complete in a single call. */ + @Test + public void testLegacyWriteToCompletes() { + assertTrue("legacyWriteTo must return true", benchmark.legacyWriteTo()); + } + + /** Legacy readFrom must complete in a single call. */ + @Test + public void testLegacyReadFromCompletes() { + assertTrue("legacyReadFrom must return true", benchmark.legacyReadFrom()); + } + + /** Round-trip for new approach preserves all fields. */ + @Test + public void testNewRoundTripPreservesFields() throws Exception { + assertTrue(benchmark.newWriteTo()); + assertTrue(benchmark.newReadFrom()); + + var src = (CacheMetricsMessage)FieldUtils.readField(benchmark, "newMsg", true); + var dst = (CacheMetricsMessage)FieldUtils.readField(benchmark, "newReadTarget", true); + + assertCacheMetricsEqual(src, dst); + } + + /** Round-trip for legacy approach preserves all fields. */ + @Test + public void testLegacyRoundTripPreservesFields() throws Exception { + assertTrue(benchmark.legacyWriteTo()); + assertTrue(benchmark.legacyReadFrom()); + + var src = (LegacyCacheMetricsMessage)FieldUtils.readField(benchmark, "legacyMsg", true); + var dst = (LegacyCacheMetricsMessage)FieldUtils.readField(benchmark, "legacyReadTarget", true); + + assertLegacyCacheMetricsEqual(src, dst); + } + + // ----- helpers ----- + + /** Asserts all 87 fields of {@link CacheMetricsMessage} are equal. */ + private static void assertCacheMetricsEqual(CacheMetricsMessage src, CacheMetricsMessage dst) { + assertEquals("cacheGets", src.cacheGets, dst.cacheGets); Review Comment: All method can be replaced with ``` assertTrue(EqualsBuilder.reflectionEquals(src, dst, false)); ``` ########## modules/benchmarks/src/main/java/org/apache/ignite/internal/benchmarks/jmh/communication/JmhCacheMetricsSerializationBenchmark.java: ########## @@ -0,0 +1,410 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.internal.benchmarks.jmh.communication; + +import java.nio.ByteBuffer; +import org.apache.ignite.internal.direct.DirectMessageReader; +import org.apache.ignite.internal.direct.DirectMessageWriter; +import org.apache.ignite.internal.managers.communication.GridIoMessageFactory; +import org.apache.ignite.internal.managers.communication.IgniteMessageFactoryImpl; +import org.apache.ignite.internal.processors.cluster.CacheMetricsMessage; +import org.apache.ignite.internal.processors.cluster.CacheMetricsMessageSerializer; +import org.apache.ignite.internal.util.typedef.internal.U; +import org.apache.ignite.plugin.extensions.communication.MessageFactory; +import org.apache.ignite.plugin.extensions.communication.MessageFactoryProvider; +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.Setup; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.Warmup; + +import static java.util.concurrent.TimeUnit.NANOSECONDS; +import static java.util.concurrent.TimeUnit.SECONDS; +import static org.apache.ignite.marshaller.Marshallers.jdk; +import static org.openjdk.jmh.annotations.Mode.AverageTime; + +/** + * Benchmark comparing legacy (inline {@code writeTo}/{@code readFrom}) vs new ({@code MessageSerializer}) + * serialization approaches for {@code CacheMetricsMessage} with 87 simple fields. + * + * <p>Run via IDE with {@link #main} or as an uber-jar: + * <pre> + * java -jar target/benchmarks.jar JmhCacheMetricsSerializationBenchmark + * </pre> + */ +@State(Scope.Thread) +@OutputTimeUnit(NANOSECONDS) +@BenchmarkMode(AverageTime) +@Warmup(iterations = 3, time = 10, timeUnit = SECONDS) +@Measurement(iterations = 5, time = 10, timeUnit = SECONDS) +public class JmhCacheMetricsSerializationBenchmark { Review Comment: A lot of duplication with `JmhNodeIdSerializationBenchmark` ########## modules/benchmarks/src/test/java/org/apache/ignite/internal/benchmarks/jmh/communication/JmhNodeIdSerializationBenchmarkTest.java: ########## @@ -0,0 +1,95 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.internal.benchmarks.jmh.communication; + +import org.apache.commons.lang3.reflect.FieldUtils; +import org.apache.ignite.spi.communication.tcp.messages.NodeIdMessage; +import org.junit.Before; +import org.junit.Test; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; + +/** + * Correctness test for {@link JmhNodeIdSerializationBenchmark}. + * + * <p>Verifies that both the new (MessageSerializer) and legacy (inline writeTo/readFrom) + * approaches produce identical round-trip results for the minimal single-field message. + */ +public class JmhNodeIdSerializationBenchmarkTest { Review Comment: A lot of duplicated code with JmhCacheMetricsSerializationBenchmarkTest ########## modules/benchmarks/src/main/java/org/apache/ignite/internal/benchmarks/jmh/communication/JmhCacheMetricsSerializationBenchmark.java: ########## @@ -0,0 +1,410 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.internal.benchmarks.jmh.communication; + +import java.nio.ByteBuffer; +import org.apache.ignite.internal.direct.DirectMessageReader; +import org.apache.ignite.internal.direct.DirectMessageWriter; +import org.apache.ignite.internal.managers.communication.GridIoMessageFactory; +import org.apache.ignite.internal.managers.communication.IgniteMessageFactoryImpl; +import org.apache.ignite.internal.processors.cluster.CacheMetricsMessage; +import org.apache.ignite.internal.processors.cluster.CacheMetricsMessageSerializer; +import org.apache.ignite.internal.util.typedef.internal.U; +import org.apache.ignite.plugin.extensions.communication.MessageFactory; +import org.apache.ignite.plugin.extensions.communication.MessageFactoryProvider; +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.Setup; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.Warmup; + +import static java.util.concurrent.TimeUnit.NANOSECONDS; +import static java.util.concurrent.TimeUnit.SECONDS; +import static org.apache.ignite.marshaller.Marshallers.jdk; +import static org.openjdk.jmh.annotations.Mode.AverageTime; + +/** + * Benchmark comparing legacy (inline {@code writeTo}/{@code readFrom}) vs new ({@code MessageSerializer}) + * serialization approaches for {@code CacheMetricsMessage} with 87 simple fields. + * + * <p>Run via IDE with {@link #main} or as an uber-jar: + * <pre> + * java -jar target/benchmarks.jar JmhCacheMetricsSerializationBenchmark + * </pre> + */ +@State(Scope.Thread) +@OutputTimeUnit(NANOSECONDS) +@BenchmarkMode(AverageTime) +@Warmup(iterations = 3, time = 10, timeUnit = SECONDS) +@Measurement(iterations = 5, time = 10, timeUnit = SECONDS) +public class JmhCacheMetricsSerializationBenchmark { + /** Buffer capacity — large enough for a fully serialized CacheMetricsMessage. */ + private static final int BUF_CAPACITY = 8 * 1024; + + // ----- New approach (MessageSerializer) ----- + + /** New-style message. */ + private CacheMetricsMessage newMsg; + + /** New-style serializer. */ + private CacheMetricsMessageSerializer newSerializer; + + /** Writer for new-style benchmarks. */ + private DirectMessageWriter newWriter; + + /** Reader for new-style benchmarks. */ + private DirectMessageReader newReader; + + /** Write buffer for new-style benchmarks. */ + private ByteBuffer newWriteBuf; + + /** Pre-filled read buffer for new-style benchmarks. */ + private ByteBuffer newReadBuf; + + /** Reusable deserialization target for new-style benchmarks. */ + private CacheMetricsMessage newReadTarget; + + // ----- Legacy approach (inline writeTo/readFrom) ----- + + /** Legacy message. */ + private LegacyCacheMetricsMessage legacyMsg; + + /** Writer for legacy benchmarks. */ + private DirectMessageWriter legacyWriter; + + /** Reader for legacy benchmarks. */ + private DirectMessageReader legacyReader; + + /** Write buffer for legacy benchmarks. */ + private ByteBuffer legacyWriteBuf; + + /** Pre-filled read buffer for legacy benchmarks. */ + private ByteBuffer legacyReadBuf; + + /** Reusable deserialization target for legacy benchmarks. */ + private LegacyCacheMetricsMessage legacyReadTarget; + + /** */ + @Setup + public void setup() throws Exception { + MessageFactory factory = new IgniteMessageFactoryImpl( + new MessageFactoryProvider[]{new GridIoMessageFactory(jdk(), U.gridClassLoader())} + ); + + // --- New approach setup --- + newMsg = buildNewMessage(); + newSerializer = new CacheMetricsMessageSerializer(); Review Comment: We can't guarantee that `CacheMetricsMessage` is not changed in master. In this case after some time in the future we can face with different fields set serialization. This issue makes test useless in master (maybe useful only for one-time testing during prototyping, without merging to master). ########## modules/benchmarks/src/test/java/org/apache/ignite/internal/benchmarks/jmh/communication/JmhCacheMetricsSerializationBenchmarkTest.java: ########## @@ -0,0 +1,217 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.internal.benchmarks.jmh.communication; + +import org.apache.commons.lang3.reflect.FieldUtils; +import org.apache.ignite.internal.processors.cluster.CacheMetricsMessage; +import org.junit.Before; +import org.junit.Test; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +/** + * Correctness test for {@link JmhCacheMetricsSerializationBenchmark}. + * + * <p>Verifies that both the new (MessageSerializer) and legacy (inline writeTo/readFrom) + * approaches produce identical round-trip results, and that all benchmark methods + * complete without errors. + */ +public class JmhCacheMetricsSerializationBenchmarkTest { Review Comment: Why do we need these two tests at all? Message equaliti is checked by core tests, invariants "... must return true" are checked in `setup()` method, and it's better to test it not in single test run, but inside this benchmark methods. For me `JmhCacheMetricsSerializationBenchmarkTest` and `JmhNodeIdSerializationBenchmarkTest` are redundant. ########## modules/benchmarks/src/main/java/org/apache/ignite/internal/benchmarks/jmh/communication/JmhCacheMetricsSerializationBenchmark.java: ########## @@ -0,0 +1,410 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.internal.benchmarks.jmh.communication; + +import java.nio.ByteBuffer; +import org.apache.ignite.internal.direct.DirectMessageReader; +import org.apache.ignite.internal.direct.DirectMessageWriter; +import org.apache.ignite.internal.managers.communication.GridIoMessageFactory; +import org.apache.ignite.internal.managers.communication.IgniteMessageFactoryImpl; +import org.apache.ignite.internal.processors.cluster.CacheMetricsMessage; +import org.apache.ignite.internal.processors.cluster.CacheMetricsMessageSerializer; +import org.apache.ignite.internal.util.typedef.internal.U; +import org.apache.ignite.plugin.extensions.communication.MessageFactory; +import org.apache.ignite.plugin.extensions.communication.MessageFactoryProvider; +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.Setup; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.Warmup; + +import static java.util.concurrent.TimeUnit.NANOSECONDS; +import static java.util.concurrent.TimeUnit.SECONDS; +import static org.apache.ignite.marshaller.Marshallers.jdk; +import static org.openjdk.jmh.annotations.Mode.AverageTime; + +/** + * Benchmark comparing legacy (inline {@code writeTo}/{@code readFrom}) vs new ({@code MessageSerializer}) + * serialization approaches for {@code CacheMetricsMessage} with 87 simple fields. + * + * <p>Run via IDE with {@link #main} or as an uber-jar: + * <pre> + * java -jar target/benchmarks.jar JmhCacheMetricsSerializationBenchmark + * </pre> + */ +@State(Scope.Thread) +@OutputTimeUnit(NANOSECONDS) +@BenchmarkMode(AverageTime) +@Warmup(iterations = 3, time = 10, timeUnit = SECONDS) +@Measurement(iterations = 5, time = 10, timeUnit = SECONDS) +public class JmhCacheMetricsSerializationBenchmark { + /** Buffer capacity — large enough for a fully serialized CacheMetricsMessage. */ + private static final int BUF_CAPACITY = 8 * 1024; + + // ----- New approach (MessageSerializer) ----- + + /** New-style message. */ + private CacheMetricsMessage newMsg; + + /** New-style serializer. */ + private CacheMetricsMessageSerializer newSerializer; + + /** Writer for new-style benchmarks. */ + private DirectMessageWriter newWriter; + + /** Reader for new-style benchmarks. */ + private DirectMessageReader newReader; + + /** Write buffer for new-style benchmarks. */ + private ByteBuffer newWriteBuf; + + /** Pre-filled read buffer for new-style benchmarks. */ + private ByteBuffer newReadBuf; + + /** Reusable deserialization target for new-style benchmarks. */ + private CacheMetricsMessage newReadTarget; + + // ----- Legacy approach (inline writeTo/readFrom) ----- + + /** Legacy message. */ + private LegacyCacheMetricsMessage legacyMsg; + + /** Writer for legacy benchmarks. */ + private DirectMessageWriter legacyWriter; + + /** Reader for legacy benchmarks. */ + private DirectMessageReader legacyReader; + + /** Write buffer for legacy benchmarks. */ + private ByteBuffer legacyWriteBuf; + + /** Pre-filled read buffer for legacy benchmarks. */ + private ByteBuffer legacyReadBuf; + + /** Reusable deserialization target for legacy benchmarks. */ + private LegacyCacheMetricsMessage legacyReadTarget; + + /** */ + @Setup + public void setup() throws Exception { + MessageFactory factory = new IgniteMessageFactoryImpl( + new MessageFactoryProvider[]{new GridIoMessageFactory(jdk(), U.gridClassLoader())} + ); + + // --- New approach setup --- + newMsg = buildNewMessage(); + newSerializer = new CacheMetricsMessageSerializer(); + newWriteBuf = ByteBuffer.allocateDirect(BUF_CAPACITY); + newReadBuf = ByteBuffer.allocateDirect(BUF_CAPACITY); + newWriter = new DirectMessageWriter(factory); + newReader = new DirectMessageReader(factory, null); + + newWriter.setBuffer(newWriteBuf); + + if (!newSerializer.writeTo(newMsg, newWriter)) + throw new IllegalStateException("Write buffer is too small for new message"); + + newWriteBuf.flip(); + newWriteBuf.position(Short.BYTES); // skip directType header + newReadBuf.put(newWriteBuf); + newReadBuf.flip(); + + newWriteBuf.clear(); + newWriter.reset(); + + newReadTarget = new CacheMetricsMessage(); + + // --- Legacy approach setup --- + legacyMsg = buildLegacyMessage(); + legacyWriteBuf = ByteBuffer.allocateDirect(BUF_CAPACITY); + legacyReadBuf = ByteBuffer.allocateDirect(BUF_CAPACITY); + legacyWriter = new DirectMessageWriter(factory); + legacyReader = new DirectMessageReader(factory, null); + + legacyWriter.setBuffer(legacyWriteBuf); + + if (!legacyMsg.writeTo(legacyWriteBuf, legacyWriter)) + throw new IllegalStateException("Write buffer is too small for legacy message"); + + legacyWriteBuf.flip(); + legacyWriteBuf.position(Short.BYTES); // skip directType header + legacyReadBuf.put(legacyWriteBuf); + legacyReadBuf.flip(); + + legacyWriteBuf.clear(); + legacyWriter.reset(); + + legacyReadTarget = new LegacyCacheMetricsMessage(); + } + + // ----- New approach benchmarks ----- + + /** Measures serialization cost using the new {@code MessageSerializer} approach. */ + @Benchmark + public boolean newWriteTo() { + newWriteBuf.clear(); + newWriter.reset(); + newWriter.setBuffer(newWriteBuf); + + return newSerializer.writeTo(newMsg, newWriter); Review Comment: The code generated to `writeTo` for legacy message is the same as the code generated to `writeTo` for serializer. What's the point of benchmarking identical code? There will never be any difference. But the part that changed (code executed before `writeTo` method) is not checked. I think it's a wrong approach. ########## modules/benchmarks/src/main/java/org/apache/ignite/internal/benchmarks/jmh/communication/JmhCacheMetricsSerializationBenchmark.java: ########## @@ -0,0 +1,410 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.internal.benchmarks.jmh.communication; + +import java.nio.ByteBuffer; +import org.apache.ignite.internal.direct.DirectMessageReader; +import org.apache.ignite.internal.direct.DirectMessageWriter; +import org.apache.ignite.internal.managers.communication.GridIoMessageFactory; +import org.apache.ignite.internal.managers.communication.IgniteMessageFactoryImpl; +import org.apache.ignite.internal.processors.cluster.CacheMetricsMessage; +import org.apache.ignite.internal.processors.cluster.CacheMetricsMessageSerializer; +import org.apache.ignite.internal.util.typedef.internal.U; +import org.apache.ignite.plugin.extensions.communication.MessageFactory; +import org.apache.ignite.plugin.extensions.communication.MessageFactoryProvider; +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.Setup; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.Warmup; + +import static java.util.concurrent.TimeUnit.NANOSECONDS; +import static java.util.concurrent.TimeUnit.SECONDS; +import static org.apache.ignite.marshaller.Marshallers.jdk; +import static org.openjdk.jmh.annotations.Mode.AverageTime; + +/** + * Benchmark comparing legacy (inline {@code writeTo}/{@code readFrom}) vs new ({@code MessageSerializer}) + * serialization approaches for {@code CacheMetricsMessage} with 87 simple fields. + * + * <p>Run via IDE with {@link #main} or as an uber-jar: + * <pre> + * java -jar target/benchmarks.jar JmhCacheMetricsSerializationBenchmark + * </pre> + */ +@State(Scope.Thread) +@OutputTimeUnit(NANOSECONDS) +@BenchmarkMode(AverageTime) +@Warmup(iterations = 3, time = 10, timeUnit = SECONDS) +@Measurement(iterations = 5, time = 10, timeUnit = SECONDS) +public class JmhCacheMetricsSerializationBenchmark { + /** Buffer capacity — large enough for a fully serialized CacheMetricsMessage. */ + private static final int BUF_CAPACITY = 8 * 1024; + + // ----- New approach (MessageSerializer) ----- + + /** New-style message. */ + private CacheMetricsMessage newMsg; + + /** New-style serializer. */ + private CacheMetricsMessageSerializer newSerializer; + + /** Writer for new-style benchmarks. */ + private DirectMessageWriter newWriter; + + /** Reader for new-style benchmarks. */ + private DirectMessageReader newReader; + + /** Write buffer for new-style benchmarks. */ + private ByteBuffer newWriteBuf; + + /** Pre-filled read buffer for new-style benchmarks. */ + private ByteBuffer newReadBuf; + + /** Reusable deserialization target for new-style benchmarks. */ + private CacheMetricsMessage newReadTarget; + + // ----- Legacy approach (inline writeTo/readFrom) ----- + + /** Legacy message. */ + private LegacyCacheMetricsMessage legacyMsg; + + /** Writer for legacy benchmarks. */ + private DirectMessageWriter legacyWriter; + + /** Reader for legacy benchmarks. */ + private DirectMessageReader legacyReader; + + /** Write buffer for legacy benchmarks. */ + private ByteBuffer legacyWriteBuf; + + /** Pre-filled read buffer for legacy benchmarks. */ + private ByteBuffer legacyReadBuf; + + /** Reusable deserialization target for legacy benchmarks. */ + private LegacyCacheMetricsMessage legacyReadTarget; + + /** */ + @Setup + public void setup() throws Exception { + MessageFactory factory = new IgniteMessageFactoryImpl( + new MessageFactoryProvider[]{new GridIoMessageFactory(jdk(), U.gridClassLoader())} + ); + + // --- New approach setup --- + newMsg = buildNewMessage(); + newSerializer = new CacheMetricsMessageSerializer(); + newWriteBuf = ByteBuffer.allocateDirect(BUF_CAPACITY); + newReadBuf = ByteBuffer.allocateDirect(BUF_CAPACITY); + newWriter = new DirectMessageWriter(factory); + newReader = new DirectMessageReader(factory, null); + + newWriter.setBuffer(newWriteBuf); + + if (!newSerializer.writeTo(newMsg, newWriter)) + throw new IllegalStateException("Write buffer is too small for new message"); + + newWriteBuf.flip(); + newWriteBuf.position(Short.BYTES); // skip directType header + newReadBuf.put(newWriteBuf); + newReadBuf.flip(); + + newWriteBuf.clear(); + newWriter.reset(); + + newReadTarget = new CacheMetricsMessage(); + + // --- Legacy approach setup --- + legacyMsg = buildLegacyMessage(); Review Comment: - As said before, all affected message lifecycle should be benchmarked, not only writeTo/readFrom part - It's not quite representative to test one instance of a message, perhaps it's better to generate some random data in messages - It's not quite representative to test only one message type (for example, new approach uses ctor.newInstance() to create message, when ctor always the same java can effectevily inline this call, but when there are several different constructors used during one benchmark, java can inline only most frequently used, and there can be some issues when not frequent constructors are used) - CacheMetricsMessage contains only primitive types, it's not a good candidate for testing. Some changes where made to nested message serialization, at least message with nested messages is better candidate. AFAIK some code for collections serialization is changed, so, message with collections should be tested too. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
