From 126571e2a20ca9beeee32c2110bc9de36e8118e1 Mon Sep 17 00:00:00 2001 From: Petr Pleshachkov <50169481+petrpleshachkov@users.noreply.github.com> Date: Fri, 1 Dec 2023 19:36:27 +0400 Subject: [PATCH 1/2] Avoid excessive garbage in map#clear() for TS [HZ-3620] --- .../map/impl/operation/ClearOperation.java | 7 ++++++- .../map/impl/operation/steps/IMapOpStep.java | 2 ++ .../impl/recordstore/DefaultRecordStore.java | 17 ++++++++++++----- 3 files changed, 20 insertions(+), 6 deletions(-) diff --git a/hazelcast/src/main/java/com/hazelcast/map/impl/operation/ClearOperation.java b/hazelcast/src/main/java/com/hazelcast/map/impl/operation/ClearOperation.java index d3bcd1a89bf3..cf38c55f5614 100644 --- a/hazelcast/src/main/java/com/hazelcast/map/impl/operation/ClearOperation.java +++ b/hazelcast/src/main/java/com/hazelcast/map/impl/operation/ClearOperation.java @@ -21,6 +21,7 @@ import com.hazelcast.map.impl.operation.steps.ClearOpSteps; import com.hazelcast.map.impl.operation.steps.engine.State; import com.hazelcast.map.impl.operation.steps.engine.Step; +import com.hazelcast.map.impl.recordstore.DefaultRecordStore; import com.hazelcast.spi.impl.operationservice.BackupAwareOperation; import com.hazelcast.spi.impl.operationservice.MutatingOperation; import com.hazelcast.spi.impl.operationservice.Operation; @@ -55,7 +56,11 @@ protected void runInternal() { @Override public Step getStartingStep() { - return ClearOpSteps.CLEAR_MEMORY; + if (recordStore == null) { + return ClearOpSteps.CLEAR_MEMORY; + } + + return ((DefaultRecordStore) recordStore).getClearOpStartingStep(); } @Override diff --git a/hazelcast/src/main/java/com/hazelcast/map/impl/operation/steps/IMapOpStep.java b/hazelcast/src/main/java/com/hazelcast/map/impl/operation/steps/IMapOpStep.java index 9cf02729104f..351e8635b011 100644 --- a/hazelcast/src/main/java/com/hazelcast/map/impl/operation/steps/IMapOpStep.java +++ b/hazelcast/src/main/java/com/hazelcast/map/impl/operation/steps/IMapOpStep.java @@ -29,6 +29,8 @@ */ public interface IMapOpStep extends Step { + int BATCH_SIZE = 1000; + /** * Decides when to offload based on configured map-store type. *

diff --git a/hazelcast/src/main/java/com/hazelcast/map/impl/recordstore/DefaultRecordStore.java b/hazelcast/src/main/java/com/hazelcast/map/impl/recordstore/DefaultRecordStore.java index 901577405447..25643bf1a2e4 100644 --- a/hazelcast/src/main/java/com/hazelcast/map/impl/recordstore/DefaultRecordStore.java +++ b/hazelcast/src/main/java/com/hazelcast/map/impl/recordstore/DefaultRecordStore.java @@ -46,6 +46,8 @@ import com.hazelcast.map.impl.mapstore.writebehind.WriteBehindStore; import com.hazelcast.map.impl.mapstore.writebehind.entry.DelayedEntry; import com.hazelcast.map.impl.operation.MapOperation; +import com.hazelcast.map.impl.operation.steps.ClearOpSteps; +import com.hazelcast.map.impl.operation.steps.engine.Step; import com.hazelcast.map.impl.querycache.QueryCacheContext; import com.hazelcast.map.impl.querycache.publisher.MapPublisherRegistry; import com.hazelcast.map.impl.querycache.publisher.PublisherContext; @@ -112,6 +114,12 @@ public class DefaultRecordStore extends AbstractEvictableRecordStore { */ protected final Collection> loadingFutures = new ConcurrentLinkedQueue<>(); + /** + * Defined by {@link com.hazelcast.spi.properties.ClusterProperty#WAN_REPLICATE_IMAP_EVICTIONS}, + * if set to true then eviction operations by this RecordStore will be WAN replicated + */ + protected boolean wanReplicateEvictions; + /** * A reference to the Json Metadata store. It is initialized lazily only if the * store is needed. @@ -131,11 +139,6 @@ public class DefaultRecordStore extends AbstractEvictableRecordStore { * key loading. */ private boolean loadedOnPreMigration; - /** - * Defined by {@link com.hazelcast.spi.properties.ClusterProperty#WAN_REPLICATE_IMAP_EVICTIONS}, - * if set to true then eviction operations by this RecordStore will be WAN replicated - */ - private boolean wanReplicateEvictions; private final IPartitionService partitionService; private final InterceptorRegistry interceptorRegistry; @@ -1642,4 +1645,8 @@ public Set getOffloadedOperations() { public boolean isTieredStorageEnabled() { return mapContainer.getMapConfig().getTieredStoreConfig().isEnabled(); } + + public Step getClearOpStartingStep() { + return ClearOpSteps.CLEAR_MEMORY; + } } From 0c7c070215fccd3e5ca89506118526da8cbf3ceb Mon Sep 17 00:00:00 2001 From: petrpleshachkov <50169481+petrpleshachkov@users.noreply.github.com> Date: Mon, 11 Dec 2023 13:03:07 +0100 Subject: [PATCH 2/2] Addressed reviewer's comments --- .../map/impl/operation/ClearOperation.java | 6 +-- .../impl/operation/steps/ClearOpSteps.java | 43 +++++++++++++++---- .../map/impl/operation/steps/IMapOpStep.java | 3 ++ .../impl/recordstore/DefaultRecordStore.java | 4 -- 4 files changed, 38 insertions(+), 18 deletions(-) diff --git a/hazelcast/src/main/java/com/hazelcast/map/impl/operation/ClearOperation.java b/hazelcast/src/main/java/com/hazelcast/map/impl/operation/ClearOperation.java index cf38c55f5614..d5bed125527c 100644 --- a/hazelcast/src/main/java/com/hazelcast/map/impl/operation/ClearOperation.java +++ b/hazelcast/src/main/java/com/hazelcast/map/impl/operation/ClearOperation.java @@ -56,11 +56,7 @@ protected void runInternal() { @Override public Step getStartingStep() { - if (recordStore == null) { - return ClearOpSteps.CLEAR_MEMORY; - } - - return ((DefaultRecordStore) recordStore).getClearOpStartingStep(); + return ClearOpSteps.CLEAR_MEMORY; } @Override diff --git a/hazelcast/src/main/java/com/hazelcast/map/impl/operation/steps/ClearOpSteps.java b/hazelcast/src/main/java/com/hazelcast/map/impl/operation/steps/ClearOpSteps.java index 3330448427f8..02216dfc90dd 100644 --- a/hazelcast/src/main/java/com/hazelcast/map/impl/operation/steps/ClearOpSteps.java +++ b/hazelcast/src/main/java/com/hazelcast/map/impl/operation/steps/ClearOpSteps.java @@ -22,9 +22,12 @@ import com.hazelcast.map.impl.operation.steps.engine.Step; import com.hazelcast.map.impl.record.Record; import com.hazelcast.map.impl.recordstore.DefaultRecordStore; +import com.hazelcast.map.impl.recordstore.RecordStore; import java.util.ArrayList; import java.util.Collection; +import java.util.Iterator; +import java.util.Map; import java.util.Set; import java.util.function.BiConsumer; @@ -45,21 +48,32 @@ public void runStep(State state) { ArrayList keys = new ArrayList<>(); ArrayList records = new ArrayList<>(); // we don't remove locked keys. These are clearable records. - recordStore.forEach(new BiConsumer<>() { - final Set lockedKeySet = recordStore.getLockStore().getLockedKeys(); - - @Override - public void accept(Data dataKey, Record record) { - if (lockedKeySet != null && !lockedKeySet.contains(dataKey)) { - keys.add(recordStore.isTieredStorageEnabled() ? toHeapData(dataKey) : dataKey); + final Set lockedKeySet = recordStore.getLockStore().getLockedKeys(); + final Iterator> iterator = recordStore.iterator(); + boolean tieredStorageEnabled = recordStore.isTieredStorageEnabled();; + + while (iterator.hasNext()) { + Map.Entry entry = iterator.next(); + Data dataKey = entry.getKey(); + Record record = entry.getValue(); + + if (lockedKeySet != null && !lockedKeySet.contains(dataKey)) { + keys.add(tieredStorageEnabled ? toHeapData(dataKey) : dataKey); + if (!recordStore.isTieredStorageEnabled()) { records.add(record); } + } + if (keys.size() == BATCH_SIZE) { + // Batch filling is completed + break; } - }, false); + } state.setKeys(keys); - state.setRecords(records); + if (!tieredStorageEnabled) { + state.setRecords(records); + } } @Override @@ -102,6 +116,17 @@ public void runStep(State state) { @Override public Step nextStep(State state) { + RecordStore recordStore = state.getRecordStore(); + int currentSize = recordStore.size(); + final Set lockedKeySet = ((DefaultRecordStore) recordStore).getLockStore().getLockedKeys(); + int lockedSize = lockedKeySet.size(); + + if (currentSize - lockedSize > 0) { + // We still have entries to process + // Process them in the next batch + return CLEAR_MEMORY; + } + return UtilSteps.FINAL_STEP; } }; diff --git a/hazelcast/src/main/java/com/hazelcast/map/impl/operation/steps/IMapOpStep.java b/hazelcast/src/main/java/com/hazelcast/map/impl/operation/steps/IMapOpStep.java index 351e8635b011..728acb8c54f6 100644 --- a/hazelcast/src/main/java/com/hazelcast/map/impl/operation/steps/IMapOpStep.java +++ b/hazelcast/src/main/java/com/hazelcast/map/impl/operation/steps/IMapOpStep.java @@ -29,6 +29,9 @@ */ public interface IMapOpStep extends Step { + /** + * The batch size of records handled in bulk operations. + */ int BATCH_SIZE = 1000; /** diff --git a/hazelcast/src/main/java/com/hazelcast/map/impl/recordstore/DefaultRecordStore.java b/hazelcast/src/main/java/com/hazelcast/map/impl/recordstore/DefaultRecordStore.java index 25643bf1a2e4..d38377a6451a 100644 --- a/hazelcast/src/main/java/com/hazelcast/map/impl/recordstore/DefaultRecordStore.java +++ b/hazelcast/src/main/java/com/hazelcast/map/impl/recordstore/DefaultRecordStore.java @@ -1645,8 +1645,4 @@ public Set getOffloadedOperations() { public boolean isTieredStorageEnabled() { return mapContainer.getMapConfig().getTieredStoreConfig().isEnabled(); } - - public Step getClearOpStartingStep() { - return ClearOpSteps.CLEAR_MEMORY; - } }