Repository: cassandra Updated Branches: refs/heads/cassandra-2.1 0396efc41 -> 88f9c92f4 refs/heads/trunk 461962866 -> 1d508f7f6
Remove misguided stack allocation optimisations patch by Benedict; reviewed by Aleksey for CASSANDRA-7515 Project: http://git-wip-us.apache.org/repos/asf/cassandra/repo Commit: http://git-wip-us.apache.org/repos/asf/cassandra/commit/88f9c92f Tree: http://git-wip-us.apache.org/repos/asf/cassandra/tree/88f9c92f Diff: http://git-wip-us.apache.org/repos/asf/cassandra/diff/88f9c92f Branch: refs/heads/cassandra-2.1 Commit: 88f9c92f4561c77b40ca25594f3b926824d506c9 Parents: 0396efc Author: Benedict Elliott Smith <[email protected]> Authored: Mon Jul 14 20:11:08 2014 +0100 Committer: Benedict Elliott Smith <[email protected]> Committed: Mon Jul 14 20:11:08 2014 +0100 ---------------------------------------------------------------------- src/java/org/apache/cassandra/db/Keyspace.java | 8 +--- src/java/org/apache/cassandra/db/Memtable.java | 2 - .../cassandra/db/commitlog/CommitLog.java | 12 +----- .../db/commitlog/CommitLogSegment.java | 42 ++++++++++---------- .../db/commitlog/CommitLogSegmentManager.java | 5 ++- 5 files changed, 27 insertions(+), 42 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/cassandra/blob/88f9c92f/src/java/org/apache/cassandra/db/Keyspace.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/db/Keyspace.java b/src/java/org/apache/cassandra/db/Keyspace.java index ff4d272..4a07f5e 100644 --- a/src/java/org/apache/cassandra/db/Keyspace.java +++ b/src/java/org/apache/cassandra/db/Keyspace.java @@ -364,18 +364,12 @@ public class Keyspace try (OpOrder.Group opGroup = writeOrder.start()) { // write the mutation to the commitlog and memtables - final ReplayPosition replayPosition; + ReplayPosition replayPosition = null; if (writeCommitLog) { Tracing.trace("Appending to commitlog"); replayPosition = CommitLog.instance.add(mutation); } - else - { - // we don't need the replayposition, but grab one anyway so that it stays stack allocated. - // (the JVM will not stack allocate if the object may be null.) - replayPosition = CommitLog.instance.getContext(); - } DecoratedKey key = StorageService.getPartitioner().decorateKey(mutation.key()); for (ColumnFamily cf : mutation.getColumnFamilies()) http://git-wip-us.apache.org/repos/asf/cassandra/blob/88f9c92f/src/java/org/apache/cassandra/db/Memtable.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/db/Memtable.java b/src/java/org/apache/cassandra/db/Memtable.java index 3c02678..1eea915 100644 --- a/src/java/org/apache/cassandra/db/Memtable.java +++ b/src/java/org/apache/cassandra/db/Memtable.java @@ -157,8 +157,6 @@ public class Memtable { // if the writeBarrier is set, we want to maintain lastReplayPosition; this is an optimisation to avoid // casing it for every write, but still ensure it is correct when writeBarrier.await() completes. - // we clone the replay position so that the object passed in does not "escape", permitting stack allocation - replayPosition = replayPosition.clone(); while (true) { ReplayPosition last = lastReplayPosition.get(); http://git-wip-us.apache.org/repos/asf/cassandra/blob/88f9c92f/src/java/org/apache/cassandra/db/commitlog/CommitLog.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/db/commitlog/CommitLog.java b/src/java/org/apache/cassandra/db/commitlog/CommitLog.java index ac1d811..a1be25d 100644 --- a/src/java/org/apache/cassandra/db/commitlog/CommitLog.java +++ b/src/java/org/apache/cassandra/db/commitlog/CommitLog.java @@ -21,12 +21,10 @@ import java.io.*; import java.lang.management.ManagementFactory; import java.nio.ByteBuffer; import java.util.*; -import java.util.concurrent.TimeUnit; import javax.management.MBeanServer; import javax.management.ObjectName; -import com.google.common.util.concurrent.Uninterruptibles; import org.apache.commons.lang3.StringUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -211,12 +209,6 @@ public class CommitLog implements CommitLogMBean */ public ReplayPosition add(Mutation mutation) { - Allocation alloc = add(mutation, new Allocation()); - return alloc.getReplayPosition(); - } - - private Allocation add(Mutation mutation, Allocation alloc) - { assert mutation != null; long size = Mutation.serializer.serializedSize(mutation, MessagingService.current_version); @@ -228,7 +220,7 @@ public class CommitLog implements CommitLogMBean totalSize, MAX_MUTATION_SIZE)); } - allocator.allocate(mutation, (int) totalSize, alloc); + Allocation alloc = allocator.allocate(mutation, (int) totalSize); try { PureJavaCrc32 checksum = new PureJavaCrc32(); @@ -256,7 +248,7 @@ public class CommitLog implements CommitLogMBean } executor.finishWriteFor(alloc); - return alloc; + return alloc.getReplayPosition(); } /** http://git-wip-us.apache.org/repos/asf/cassandra/blob/88f9c92f/src/java/org/apache/cassandra/db/commitlog/CommitLogSegment.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/db/commitlog/CommitLogSegment.java b/src/java/org/apache/cassandra/db/commitlog/CommitLogSegment.java index a983f01..6f756f7 100644 --- a/src/java/org/apache/cassandra/db/commitlog/CommitLogSegment.java +++ b/src/java/org/apache/cassandra/db/commitlog/CommitLogSegment.java @@ -43,7 +43,6 @@ import org.apache.cassandra.config.DatabaseDescriptor; import org.apache.cassandra.config.Schema; import org.apache.cassandra.db.ColumnFamily; import org.apache.cassandra.db.Mutation; -import org.apache.cassandra.io.FSReadError; import org.apache.cassandra.io.FSWriteError; import org.apache.cassandra.io.util.FileUtils; import org.apache.cassandra.utils.CLibrary; @@ -180,31 +179,26 @@ public class CommitLogSegment } /** - * allocate space in this buffer for the provided mutation, and populate the provided - * Allocation object, returning true on success. False indicates there is not enough room in - * this segment, and a new segment is needed + * Allocate space in this buffer for the provided mutation, and return the allocated Allocation object. + * Returns null if there is not enough space in this segment, and a new segment is needed. */ - boolean allocate(Mutation mutation, int size, Allocation alloc) + Allocation allocate(Mutation mutation, int size) { - final OpOrder.Group commandOrder = appendOrder.start(); + final OpOrder.Group opGroup = appendOrder.start(); try { int position = allocate(size); if (position < 0) { - commandOrder.close(); - return false; + opGroup.close(); + return null; } - alloc.buffer = (ByteBuffer) buffer.duplicate().position(position).limit(position + size); - alloc.position = position; - alloc.segment = this; - alloc.appendOp = commandOrder; markDirty(mutation, position); - return true; + return new Allocation(this, opGroup, position, (ByteBuffer) buffer.duplicate().position(position).limit(position + size)); } catch (Throwable t) { - commandOrder.close(); + opGroup.close(); throw t; } } @@ -587,10 +581,18 @@ public class CommitLogSegment static class Allocation { - private CommitLogSegment segment; - private OpOrder.Group appendOp; - private int position; - private ByteBuffer buffer; + private final CommitLogSegment segment; + private final OpOrder.Group appendOp; + private final int position; + private final ByteBuffer buffer; + + Allocation(CommitLogSegment segment, OpOrder.Group appendOp, int position, ByteBuffer buffer) + { + this.segment = segment; + this.appendOp = appendOp; + this.position = position; + this.buffer = buffer; + } CommitLogSegment getSegment() { @@ -623,9 +625,7 @@ public class CommitLogSegment public ReplayPosition getReplayPosition() { - // always allocate a ReplayPosition to let stack allocation do its magic. If we return null, we always - // have to allocate an object on the stack - return new ReplayPosition(segment == null ? -1 : segment.id, segment == null ? 0 : buffer.limit()); + return new ReplayPosition(segment.id, buffer.limit()); } } http://git-wip-us.apache.org/repos/asf/cassandra/blob/88f9c92f/src/java/org/apache/cassandra/db/commitlog/CommitLogSegmentManager.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/db/commitlog/CommitLogSegmentManager.java b/src/java/org/apache/cassandra/db/commitlog/CommitLogSegmentManager.java index e1a7e39..c93d1eb 100644 --- a/src/java/org/apache/cassandra/db/commitlog/CommitLogSegmentManager.java +++ b/src/java/org/apache/cassandra/db/commitlog/CommitLogSegmentManager.java @@ -176,11 +176,12 @@ public class CommitLogSegmentManager * * @return the provided Allocation object */ - public Allocation allocate(Mutation mutation, int size, Allocation alloc) + public Allocation allocate(Mutation mutation, int size) { CommitLogSegment segment = allocatingFrom(); - while (!segment.allocate(mutation, size, alloc)) + Allocation alloc; + while ( null == (alloc = segment.allocate(mutation, size)) ) { // failed to allocate, so move to a new segment with enough room advanceAllocatingFrom(segment);
