From 6580017b9c480d0ea0950113ce67b1b2ebe9bad7 Mon Sep 17 00:00:00 2001 From: leventov Date: Thu, 23 Jul 2015 21:55:49 +0300 Subject: [PATCH] Lock ordering made right --- README.md | 58 +++++++++++-------- .../chronicle/hash/HashQueryContext.java | 14 ++++- .../hash/{Segment.java => SegmentLock.java} | 24 +++++--- .../hash/impl/stage/entry/SegmentStages.java | 4 +- .../chronicle/map/MapQueryContext.java | 2 - .../chronicle/map/NestedContextsTest.java | 57 ++++++++++-------- 6 files changed, 100 insertions(+), 59 deletions(-) rename src/main/java/net/openhft/chronicle/hash/{Segment.java => SegmentLock.java} (56%) diff --git a/README.md b/README.md index 2b3a3430c..cf8e5f0e0 100644 --- a/README.md +++ b/README.md @@ -1926,18 +1926,22 @@ Using nested contexts: ChronicleMap> graph, int source, int target) { if (source == target) throw new IllegalArgumentException("loops are forbidden"); - + ExternalMapQueryContext, ?> sourceC = graph.queryContext(source); + ExternalMapQueryContext, ?> targetC = graph.queryContext(target); // order for consistent lock acquisition => avoid dead lock - if (target < source) { - int t = source; - source = target; - target = t; + if (sourceC.segmentIndex() <= targetC.segmentIndex()) { + return innerAddEdge(source, sourceC, target, targetC); + } else { + return innerAddEdge(target, targetC, source, sourceC); } + } - try (ExternalMapQueryContext, ?> sc = graph.queryContext(source)) { - sc.updateLock().lock(); - try (ExternalMapQueryContext, ?> tc = - graph.queryContext(target)) { + private static boolean innerAddEdge( + int source, ExternalMapQueryContext, ?> sourceContext, + int target, ExternalMapQueryContext, ?> targetContext) { + try (ExternalMapQueryContext, ?> sc = sourceContext) { + try (ExternalMapQueryContext, ?> tc = targetContext) { + sc.updateLock().lock(); tc.updateLock().lock(); MapEntry> sEntry = sc.entry(); if (sEntry != null) { @@ -2007,23 +2011,29 @@ Using nested contexts: public static boolean removeEdge( ChronicleMap> graph, int source, int target) { + ExternalMapQueryContext, ?> sourceC = graph.queryContext(source); + ExternalMapQueryContext, ?> targetC = graph.queryContext(target); // order for consistent lock acquisition => avoid dead lock - if (target < source) { - int t = source; - source = target; - target = t; + if (sourceC.segmentIndex() <= targetC.segmentIndex()) { + return innerRemoveEdge(source, sourceC, target, targetC); + } else { + return innerRemoveEdge(target, targetC, source, sourceC); } - - try (ExternalMapQueryContext, ?> sc = graph.queryContext(source)) { - sc.updateLock().lock(); - MapEntry> sEntry = sc.entry(); - if (sEntry == null) - return false; - Set sNeighbours = sEntry.value().get(); - if (!sNeighbours.remove(target)) - return false; - try (ExternalMapQueryContext, ?> tc = - graph.queryContext(target)) { + } + + private static boolean innerRemoveEdge( + int source, ExternalMapQueryContext, ?> sourceContext, + int target, ExternalMapQueryContext, ?> targetContext) { + try (ExternalMapQueryContext, ?> sc = sourceContext) { + try (ExternalMapQueryContext, ?> tc = targetContext) { + sc.updateLock().lock(); + MapEntry> sEntry = sc.entry(); + if (sEntry == null) + return false; + Set sNeighbours = sEntry.value().get(); + if (!sNeighbours.remove(target)) + return false; + tc.updateLock().lock(); MapEntry> tEntry = tc.entry(); if (tEntry == null) diff --git a/src/main/java/net/openhft/chronicle/hash/HashQueryContext.java b/src/main/java/net/openhft/chronicle/hash/HashQueryContext.java index 622c9fe2f..011065bfa 100644 --- a/src/main/java/net/openhft/chronicle/hash/HashQueryContext.java +++ b/src/main/java/net/openhft/chronicle/hash/HashQueryContext.java @@ -7,6 +7,7 @@ import net.openhft.chronicle.map.MapMethods; import net.openhft.chronicle.map.MapQueryContext; import net.openhft.chronicle.set.SetEntryOperations; import net.openhft.chronicle.set.SetQueryContext; +import org.jetbrains.annotations.Nullable; /** * Context of {@link ChronicleHash} operations with individual keys. @@ -137,7 +138,17 @@ import net.openhft.chronicle.set.SetQueryContext; * @param the hash key type * @see ChronicleHash#queryContext(Object) */ -public interface HashQueryContext extends HashContext, InterProcessReadWriteUpdateLock { +public interface HashQueryContext extends HashContext, SegmentLock { + + /** + * Returns the index of the accessed segment, where the queried key is located (or to which + * the key is going to be put). + * + *

This index might also be used as the {@code InterProcessReadWriteUpdateLock} identifier, + * because {@code ChronicleHashes} has per-segment locks. + */ + @Override + int segmentIndex(); /** * Returns the queried key as a {@code Data}. @@ -160,5 +171,6 @@ public interface HashQueryContext extends HashContext, InterProcessReadWri * @implNote Might acquire {@link #readLock} before searching for the key, if the context * is not locked yet. */ + @Nullable HashAbsentEntry absentEntry(); } diff --git a/src/main/java/net/openhft/chronicle/hash/Segment.java b/src/main/java/net/openhft/chronicle/hash/SegmentLock.java similarity index 56% rename from src/main/java/net/openhft/chronicle/hash/Segment.java rename to src/main/java/net/openhft/chronicle/hash/SegmentLock.java index b13e2619e..d8b408232 100644 --- a/src/main/java/net/openhft/chronicle/hash/Segment.java +++ b/src/main/java/net/openhft/chronicle/hash/SegmentLock.java @@ -1,5 +1,7 @@ /* - * Copyright 2014 Higher Frequency Trading http://www.higherfrequencytrading.com + * Copyright 2015 Higher Frequency Trading + * + * http://www.higherfrequencytrading.com * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -17,11 +19,19 @@ package net.openhft.chronicle.hash; import net.openhft.chronicle.hash.locks.InterProcessReadWriteUpdateLock; -import org.jetbrains.annotations.NotNull; -public interface Segment { - long index(); - - @NotNull - InterProcessReadWriteUpdateLock lock(); +/** + * {@link InterProcessReadWriteUpdateLock} of a segment in {@code ChronicleHash}. + * + *

In Chronicle-Map off-heap design, locks (and concurrency) are per-segment. + * + * @see ChronicleHashBuilder#minSegments(int) + * @see ChronicleHashBuilder#actualSegments(int) + */ +public interface SegmentLock extends InterProcessReadWriteUpdateLock { + + /** + * Returns the index of the accessed segment. + */ + int segmentIndex(); } diff --git a/src/main/java/net/openhft/chronicle/hash/impl/stage/entry/SegmentStages.java b/src/main/java/net/openhft/chronicle/hash/impl/stage/entry/SegmentStages.java index 1c555b5d6..b97e77f41 100644 --- a/src/main/java/net/openhft/chronicle/hash/impl/stage/entry/SegmentStages.java +++ b/src/main/java/net/openhft/chronicle/hash/impl/stage/entry/SegmentStages.java @@ -18,6 +18,7 @@ package net.openhft.chronicle.hash.impl.stage.entry; +import net.openhft.chronicle.hash.SegmentLock; import net.openhft.chronicle.hash.impl.BigSegmentHeader; import net.openhft.chronicle.hash.impl.LocalLockState; import net.openhft.chronicle.hash.impl.SegmentHeader; @@ -38,7 +39,7 @@ import org.jetbrains.annotations.NotNull; import static net.openhft.chronicle.hash.impl.LocalLockState.UNLOCKED; @Staged -public abstract class SegmentStages implements InterProcessReadWriteUpdateLock { +public abstract class SegmentStages implements SegmentLock { @StageRef Chaining chaining; @StageRef public VanillaChronicleHashHolder hh; @@ -48,7 +49,6 @@ public abstract class SegmentStages implements InterProcessReadWriteUpdateLock { public void initTheSegmentIndex(int segmentIndex) { this.segmentIndex = segmentIndex; - } // "SegHeader" because stage-generator doesn't support stage names - one is a prefix of another diff --git a/src/main/java/net/openhft/chronicle/map/MapQueryContext.java b/src/main/java/net/openhft/chronicle/map/MapQueryContext.java index 05c534330..8843c877d 100644 --- a/src/main/java/net/openhft/chronicle/map/MapQueryContext.java +++ b/src/main/java/net/openhft/chronicle/map/MapQueryContext.java @@ -27,8 +27,6 @@ import org.jetbrains.annotations.Nullable; * (like during {@code get()}, {@code put()}, etc., opposed to bulk operations). * This is the main context type of {@link MapMethods} and {@link MapRemoteOperations}. * - * - * * @param the map key type * @param the map value type * @param the return type of {@link MapEntryOperations} specialized for the queried map diff --git a/src/test/java/net/openhft/chronicle/map/NestedContextsTest.java b/src/test/java/net/openhft/chronicle/map/NestedContextsTest.java index 88c3f574d..8c6b801ce 100644 --- a/src/test/java/net/openhft/chronicle/map/NestedContextsTest.java +++ b/src/test/java/net/openhft/chronicle/map/NestedContextsTest.java @@ -37,7 +37,7 @@ public class NestedContextsTest { public void nestedContextsTest() throws ExecutionException, InterruptedException { ChronicleMap> graph = ChronicleMapBuilder .of(Integer.class, (Class>) (Class) Set.class) - .actualSegments(1) + .actualSegments(2) .create(); addEdge(graph, 1, 2); @@ -81,17 +81,22 @@ public class NestedContextsTest { ChronicleMap> graph, int source, int target) { if (source == target) throw new IllegalArgumentException("loops are forbidden"); + ExternalMapQueryContext, ?> sourceC = graph.queryContext(source); + ExternalMapQueryContext, ?> targetC = graph.queryContext(target); // order for consistent lock acquisition => avoid dead lock - if (target < source) { - int t = source; - source = target; - target = t; + if (sourceC.segmentIndex() <= targetC.segmentIndex()) { + return innerAddEdge(source, sourceC, target, targetC); + } else { + return innerAddEdge(target, targetC, source, sourceC); } + } - try (ExternalMapQueryContext, ?> sc = graph.queryContext(source)) { - sc.updateLock().lock(); - try (ExternalMapQueryContext, ?> tc = - graph.queryContext(target)) { + private static boolean innerAddEdge( + int source, ExternalMapQueryContext, ?> sourceContext, + int target, ExternalMapQueryContext, ?> targetContext) { + try (ExternalMapQueryContext, ?> sc = sourceContext) { + try (ExternalMapQueryContext, ?> tc = targetContext) { + sc.updateLock().lock(); tc.updateLock().lock(); MapEntry> sEntry = sc.entry(); if (sEntry != null) { @@ -161,23 +166,29 @@ public class NestedContextsTest { public static boolean removeEdge( ChronicleMap> graph, int source, int target) { + ExternalMapQueryContext, ?> sourceC = graph.queryContext(source); + ExternalMapQueryContext, ?> targetC = graph.queryContext(target); // order for consistent lock acquisition => avoid dead lock - if (target < source) { - int t = source; - source = target; - target = t; + if (sourceC.segmentIndex() <= targetC.segmentIndex()) { + return innerRemoveEdge(source, sourceC, target, targetC); + } else { + return innerRemoveEdge(target, targetC, source, sourceC); } + } + + private static boolean innerRemoveEdge( + int source, ExternalMapQueryContext, ?> sourceContext, + int target, ExternalMapQueryContext, ?> targetContext) { + try (ExternalMapQueryContext, ?> sc = sourceContext) { + try (ExternalMapQueryContext, ?> tc = targetContext) { + sc.updateLock().lock(); + MapEntry> sEntry = sc.entry(); + if (sEntry == null) + return false; + Set sNeighbours = sEntry.value().get(); + if (!sNeighbours.remove(target)) + return false; - try (ExternalMapQueryContext, ?> sc = graph.queryContext(source)) { - sc.updateLock().lock(); - MapEntry> sEntry = sc.entry(); - if (sEntry == null) - return false; - Set sNeighbours = sEntry.value().get(); - if (!sNeighbours.remove(target)) - return false; - try (ExternalMapQueryContext, ?> tc = - graph.queryContext(target)) { tc.updateLock().lock(); MapEntry> tEntry = tc.entry(); if (tEntry == null)