From a0eb9a2f98e1f6f6cc2ec3461f2ce53de6a63885 Mon Sep 17 00:00:00 2001 From: leventov Date: Thu, 30 Jul 2015 03:15:31 +0300 Subject: [PATCH] Fixed a bug in MapEntryStages.putValueDeletedEntry() -- the slot in hashLookup is empty, should be filled, not just the value updated --- .../chronicle/map/impl/MapQueryContext.java | 3 +- .../map/impl/stage/entry/MapEntryStages.java | 52 ++----------- .../stage/entry/ReplicatedMapEntryStages.java | 5 -- .../impl/stage/query/QueryMapEntryStages.java | 78 +++++++++++++++++++ .../chronicle/map/DeletedSearchStateTest.java | 38 +++++++++ 5 files changed, 122 insertions(+), 54 deletions(-) create mode 100644 src/main/java/net/openhft/chronicle/map/impl/stage/query/QueryMapEntryStages.java create mode 100644 src/test/java/net/openhft/chronicle/map/DeletedSearchStateTest.java diff --git a/src/main/java/net/openhft/chronicle/map/impl/MapQueryContext.java b/src/main/java/net/openhft/chronicle/map/impl/MapQueryContext.java index f6a65e204..631fefe1b 100644 --- a/src/main/java/net/openhft/chronicle/map/impl/MapQueryContext.java +++ b/src/main/java/net/openhft/chronicle/map/impl/MapQueryContext.java @@ -36,7 +36,6 @@ import net.openhft.chronicle.map.impl.stage.data.bytes.InputFirstValueBytesData; import net.openhft.chronicle.map.impl.stage.data.bytes.InputSecondValueBytesData; import net.openhft.chronicle.map.impl.stage.data.instance.InputValueInstanceData; import net.openhft.chronicle.map.impl.stage.data.instance.WrappedValueInstanceData; -import net.openhft.chronicle.map.impl.stage.entry.MapEntryStages; import net.openhft.chronicle.map.impl.stage.input.MapInputBytesValues; import net.openhft.chronicle.map.impl.stage.map.MapEntryOperationsDelegation; import net.openhft.chronicle.map.impl.stage.map.ValueBytesInterop; @@ -66,7 +65,7 @@ import net.openhft.sg.Staged; QueryCheckOnEachPublicOperation.class, SearchAllocatedChunks.class, - MapEntryStages.class, + QueryMapEntryStages.class, MapEntryOperationsDelegation.class, WrappedValueInstanceValueHolder.class, MapQuery.class, diff --git a/src/main/java/net/openhft/chronicle/map/impl/stage/entry/MapEntryStages.java b/src/main/java/net/openhft/chronicle/map/impl/stage/entry/MapEntryStages.java index 952ac7bb2..ab760dc76 100644 --- a/src/main/java/net/openhft/chronicle/map/impl/stage/entry/MapEntryStages.java +++ b/src/main/java/net/openhft/chronicle/map/impl/stage/entry/MapEntryStages.java @@ -33,10 +33,11 @@ import org.jetbrains.annotations.NotNull; public abstract class MapEntryStages extends HashEntryStages implements MapEntry { - @StageRef VanillaChronicleMapHolder mh; - @StageRef AllocatedChunks allocatedChunks; + @StageRef public VanillaChronicleMapHolder mh; + @StageRef public AllocatedChunks allocatedChunks; @StageRef HashLookup hashLookup; + long countValueSizeOffset() { return keyEnd(); } @@ -106,51 +107,8 @@ public abstract class MapEntryStages extends HashEntryStages } public void putValueDeletedEntry(Data newValue) { - assert s.innerUpdateLock.isHeldByCurrentThread(); - - int newSizeInChunks; - long entryStartOffset = keySizeOffset; - long newSizeOfEverythingBeforeValue = -1; - boolean newValueSizeIsDifferent = newValue.size() != valueSize; - if (newValueSizeIsDifferent) { - newSizeOfEverythingBeforeValue = newSizeOfEverythingBeforeValue(newValue); - long newValueOffset = mh.m().alignment.alignAddr( - entryStartOffset + newSizeOfEverythingBeforeValue); - long newEntrySize = newValueOffset + newValue.size() - entryStartOffset; - newSizeInChunks = mh.m().inChunks(newEntrySize); - } else { - newSizeInChunks = entrySizeInChunks; - } - if (pos + newSizeInChunks < s.freeList.size() && - s.freeList.allClear(pos, pos + newSizeInChunks)) { - s.freeList.set(pos, pos + newSizeInChunks); - s.innerWriteLock.lock(); - allocatedChunks.incrementSegmentEntriesIfNeeded(); - if (newValueSizeIsDifferent) { - initValue(newValue); - } else { - writeValue(newValue); - } - } else { - if (newValueSizeIsDifferent) { - assert newSizeOfEverythingBeforeValue >= 0; - } else { - newSizeOfEverythingBeforeValue = newSizeOfEverythingBeforeValue(newValue); - } - long entrySize = innerEntrySize(newSizeOfEverythingBeforeValue, newValue.size()); - if (newValueSizeIsDifferent) { - allocatedChunks.initEntryAndKeyCopying(entrySize, valueSizeOffset - entryStartOffset); - initValue(newValue); - } else { - long oldValueSizeOffset = valueSizeOffset; - long oldValueSize = valueSize; - long oldValueOffset = valueOffset; - allocatedChunks.initEntryAndKeyCopying(entrySize, valueOffset - entryStartOffset); - initValueWithoutSize(newValue, oldValueSizeOffset, oldValueSize, oldValueOffset); - } - freeExtraAllocatedChunks(); - } - hashLookup.putValueVolatile(hlp.hashLookupPos, pos); + throw new AssertionError("putValueDeletedEntry() might be called only from " + + "non-Replicated Map query context"); } public void writeValueAndPutPos(Data value) { diff --git a/src/main/java/net/openhft/chronicle/map/impl/stage/entry/ReplicatedMapEntryStages.java b/src/main/java/net/openhft/chronicle/map/impl/stage/entry/ReplicatedMapEntryStages.java index 9d9d4e27e..e46be08b9 100644 --- a/src/main/java/net/openhft/chronicle/map/impl/stage/entry/ReplicatedMapEntryStages.java +++ b/src/main/java/net/openhft/chronicle/map/impl/stage/entry/ReplicatedMapEntryStages.java @@ -158,9 +158,4 @@ public abstract class ReplicatedMapEntryStages extends MapEntryStages newValue) { - throw new AssertionError("Replicated Map doesn't remove entries truly, yet"); - } } diff --git a/src/main/java/net/openhft/chronicle/map/impl/stage/query/QueryMapEntryStages.java b/src/main/java/net/openhft/chronicle/map/impl/stage/query/QueryMapEntryStages.java new file mode 100644 index 000000000..f877566bb --- /dev/null +++ b/src/main/java/net/openhft/chronicle/map/impl/stage/query/QueryMapEntryStages.java @@ -0,0 +1,78 @@ +/* + * Copyright (C) 2015 higherfrequencytrading.com + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Lesser General Public License as published by + * the Free Software Foundation, either version 3 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program. If not, see . + */ + +package net.openhft.chronicle.map.impl.stage.query; + +import net.openhft.chronicle.hash.Data; +import net.openhft.chronicle.hash.impl.stage.query.HashLookupSearch; +import net.openhft.chronicle.map.impl.stage.entry.MapEntryStages; +import net.openhft.sg.StageRef; +import net.openhft.sg.Staged; + +@Staged +public abstract class QueryMapEntryStages extends MapEntryStages { + + @StageRef HashLookupSearch hls; + + @Override + public void putValueDeletedEntry(Data newValue) { + assert s.innerUpdateLock.isHeldByCurrentThread(); + + int newSizeInChunks; + long entryStartOffset = keySizeOffset; + long newSizeOfEverythingBeforeValue = -1; + boolean newValueSizeIsDifferent = newValue.size() != valueSize; + if (newValueSizeIsDifferent) { + newSizeOfEverythingBeforeValue = newSizeOfEverythingBeforeValue(newValue); + long newValueOffset = mh.m().alignment.alignAddr( + entryStartOffset + newSizeOfEverythingBeforeValue); + long newEntrySize = newValueOffset + newValue.size() - entryStartOffset; + newSizeInChunks = mh.m().inChunks(newEntrySize); + } else { + newSizeInChunks = entrySizeInChunks; + } + if (pos + newSizeInChunks < s.freeList.size() && + s.freeList.allClear(pos, pos + newSizeInChunks)) { + s.freeList.set(pos, pos + newSizeInChunks); + s.innerWriteLock.lock(); + allocatedChunks.incrementSegmentEntriesIfNeeded(); + if (newValueSizeIsDifferent) { + initValue(newValue); + } else { + writeValue(newValue); + } + } else { + if (newValueSizeIsDifferent) { + assert newSizeOfEverythingBeforeValue >= 0; + } else { + newSizeOfEverythingBeforeValue = newSizeOfEverythingBeforeValue(newValue); + } + long entrySize = innerEntrySize(newSizeOfEverythingBeforeValue, newValue.size()); + if (newValueSizeIsDifferent) { + allocatedChunks.initEntryAndKeyCopying(entrySize, valueSizeOffset - entryStartOffset); + initValue(newValue); + } else { + long oldValueSizeOffset = valueSizeOffset; + long oldValueSize = valueSize; + long oldValueOffset = valueOffset; + allocatedChunks.initEntryAndKeyCopying(entrySize, valueOffset - entryStartOffset); + initValueWithoutSize(newValue, oldValueSizeOffset, oldValueSize, oldValueOffset); + } + freeExtraAllocatedChunks(); + } + hls.putNewVolatile(pos); + } +} diff --git a/src/test/java/net/openhft/chronicle/map/DeletedSearchStateTest.java b/src/test/java/net/openhft/chronicle/map/DeletedSearchStateTest.java new file mode 100644 index 000000000..6ccf0fc56 --- /dev/null +++ b/src/test/java/net/openhft/chronicle/map/DeletedSearchStateTest.java @@ -0,0 +1,38 @@ +/* + * Copyright (C) 2015 higherfrequencytrading.com + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Lesser General Public License as published by + * the Free Software Foundation, either version 3 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program. If not, see . + */ + +package net.openhft.chronicle.map; + +import org.junit.Assert; +import org.junit.Test; + +public class DeletedSearchStateTest { + + @Test + public void deletedSearchStateTest() { + ChronicleMap map = + ChronicleMapBuilder.of(Integer.class, Integer.class).entries(100).create(); + + try (ExternalMapQueryContext q = map.queryContext(42)) { + q.updateLock().lock(); + q.insert(q.absentEntry(), q.wrapValueAsData(1)); + q.remove(q.entry()); + q.insert(q.absentEntry(), q.wrapValueAsData(2)); + } + + Assert.assertEquals((Integer) 2, map.get(42)); + } +}