From 2d991a7e4cc73b522f2c8adf14b5ff37a7947427 Mon Sep 17 00:00:00 2001 From: Sylvain Lebresne Date: Thu, 24 Sep 2015 14:09:21 -0700 Subject: [PATCH] Minor optimizations patch by slebresne; reviewed by blambov for CASSANDRA-10410 The patch includes the 3 minor optimizations desribed below: 1) Faster copy in CompositeType.build(). We call CompositeType.build() relatively often when a table has a composite partition key. When copying source buffers into the composite result in that method, we can use our existing ByteBuffer.arrayCopy() method which is supposed to be faster since it uses FastByteOperations and saves the duplication of the buffer to copy 2) Faster UUID->ByteBuffer. We convert UUID to their ByteBuffer representation in a few places and do that by converting the UUID to a byte[] and then wrapping it. But our convertion of UUID to byte[] writes the 2 long the UUID is composed of byte by byte, while for a ByteBuffer we can easily just "put" both long. So the patch introduces a specific method to do that. That's arguably cleaner anyway. 3) Memoize hash value for DataResource. Every request does a hash map lookup on a DataResource (for validating access rights) which imply calling DataResource.hashCode(). Currently that uses Objects.hashCode(), which is a varargs methods and allocate an array (which, according to MissionControl seems to not be stack allocated). The patch does 2 things: it computes the hash only once in the ctor and it stores the DataResource for each table in the CFMetaData and uses that when checking access permissions for SelectStatement and ModificationStatement (the 2 statements where performance matters). The 2nd point also saves some unecessary test that the table exists when we actually know it does. --- .../apache/cassandra/auth/DataResource.java | 29 +++++++------------ .../apache/cassandra/config/CFMetaData.java | 4 +++ .../cassandra/cql3/functions/TimeFcts.java | 4 +-- .../statements/ModificationStatement.java | 8 ++--- .../cql3/statements/SelectStatement.java | 4 +-- .../cassandra/db/marshal/CompositeType.java | 4 ++- .../apache/cassandra/db/marshal/UUIDType.java | 2 +- .../cassandra/serializers/UUIDSerializer.java | 2 +- .../apache/cassandra/service/ClientState.java | 7 +++++ .../org/apache/cassandra/utils/UUIDGen.java | 9 ++++++ .../org/apache/cassandra/utils/UUIDTests.java | 10 ++++++- 11 files changed, 52 insertions(+), 31 deletions(-) diff --git a/src/java/org/apache/cassandra/auth/DataResource.java b/src/java/org/apache/cassandra/auth/DataResource.java index f64ed93645..0aa24db8cc 100644 --- a/src/java/org/apache/cassandra/auth/DataResource.java +++ b/src/java/org/apache/cassandra/auth/DataResource.java @@ -54,31 +54,22 @@ public class DataResource implements IResource Permission.MODIFY, Permission.AUTHORIZE); private static final String ROOT_NAME = "data"; - private static final DataResource ROOT_RESOURCE = new DataResource(); + private static final DataResource ROOT_RESOURCE = new DataResource(Level.ROOT, null, null); private final Level level; private final String keyspace; private final String table; - private DataResource() - { - level = Level.ROOT; - keyspace = null; - table = null; - } + // memoized hashcode since DataRessource is immutable and used in hashmaps often + private final transient int hash; - private DataResource(String keyspace) + private DataResource(Level level, String keyspace, String table) { - level = Level.KEYSPACE; - this.keyspace = keyspace; - table = null; - } - - private DataResource(String keyspace, String table) - { - level = Level.TABLE; + this.level = level; this.keyspace = keyspace; this.table = table; + + this.hash = Objects.hashCode(level, keyspace, table); } /** @@ -97,7 +88,7 @@ public class DataResource implements IResource */ public static DataResource keyspace(String keyspace) { - return new DataResource(keyspace); + return new DataResource(Level.KEYSPACE, keyspace, null); } /** @@ -109,7 +100,7 @@ public class DataResource implements IResource */ public static DataResource table(String keyspace, String table) { - return new DataResource(keyspace, table); + return new DataResource(Level.TABLE, keyspace, table); } /** @@ -272,6 +263,6 @@ public class DataResource implements IResource @Override public int hashCode() { - return Objects.hashCode(level, keyspace, table); + return hash; } } diff --git a/src/java/org/apache/cassandra/config/CFMetaData.java b/src/java/org/apache/cassandra/config/CFMetaData.java index 00ca70488b..5db1985a98 100644 --- a/src/java/org/apache/cassandra/config/CFMetaData.java +++ b/src/java/org/apache/cassandra/config/CFMetaData.java @@ -38,6 +38,7 @@ import org.apache.commons.lang3.builder.ToStringBuilder; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import org.apache.cassandra.auth.DataResource; import org.apache.cassandra.cql3.ColumnIdentifier; import org.apache.cassandra.cql3.QueryProcessor; import org.apache.cassandra.cql3.statements.CFStatement; @@ -118,6 +119,8 @@ public final class CFMetaData // for those tables in practice). private volatile ColumnDefinition compactValueColumn; + public final DataResource resource; + /* * All of these methods will go away once CFMetaData becomes completely immutable. */ @@ -288,6 +291,7 @@ public final class CFMetaData this.partitionColumns = partitionColumns; this.serializers = new Serializers(this); + this.resource = DataResource.table(ksName, cfName); rebuild(); } diff --git a/src/java/org/apache/cassandra/cql3/functions/TimeFcts.java b/src/java/org/apache/cassandra/cql3/functions/TimeFcts.java index 93d6d3bdf4..1b87261b3b 100644 --- a/src/java/org/apache/cassandra/cql3/functions/TimeFcts.java +++ b/src/java/org/apache/cassandra/cql3/functions/TimeFcts.java @@ -67,7 +67,7 @@ public abstract class TimeFcts if (bb == null) return null; - return ByteBuffer.wrap(UUIDGen.decompose(UUIDGen.minTimeUUID(TimestampType.instance.compose(bb).getTime()))); + return UUIDGen.toByteBuffer(UUIDGen.minTimeUUID(TimestampType.instance.compose(bb).getTime())); } }; @@ -79,7 +79,7 @@ public abstract class TimeFcts if (bb == null) return null; - return ByteBuffer.wrap(UUIDGen.decompose(UUIDGen.maxTimeUUID(TimestampType.instance.compose(bb).getTime()))); + return UUIDGen.toByteBuffer(UUIDGen.maxTimeUUID(TimestampType.instance.compose(bb).getTime())); } }; diff --git a/src/java/org/apache/cassandra/cql3/statements/ModificationStatement.java b/src/java/org/apache/cassandra/cql3/statements/ModificationStatement.java index 23a26d0db7..7eefd8e275 100644 --- a/src/java/org/apache/cassandra/cql3/statements/ModificationStatement.java +++ b/src/java/org/apache/cassandra/cql3/statements/ModificationStatement.java @@ -193,21 +193,21 @@ public abstract class ModificationStatement implements CQLStatement public void checkAccess(ClientState state) throws InvalidRequestException, UnauthorizedException { - state.hasColumnFamilyAccess(keyspace(), columnFamily(), Permission.MODIFY); + state.hasColumnFamilyAccess(cfm, Permission.MODIFY); // CAS updates can be used to simulate a SELECT query, so should require Permission.SELECT as well. if (hasConditions()) - state.hasColumnFamilyAccess(keyspace(), columnFamily(), Permission.SELECT); + state.hasColumnFamilyAccess(cfm, Permission.SELECT); // MV updates need to get the current state from the table, and might update the views // Require Permission.SELECT on the base table, and Permission.MODIFY on the views Iterator views = View.findAll(keyspace(), columnFamily()).iterator(); if (views.hasNext()) { - state.hasColumnFamilyAccess(keyspace(), columnFamily(), Permission.SELECT); + state.hasColumnFamilyAccess(cfm, Permission.SELECT); do { - state.hasColumnFamilyAccess(keyspace(), views.next().viewName, Permission.MODIFY); + state.hasColumnFamilyAccess(views.next().metadata, Permission.MODIFY); } while (views.hasNext()); } diff --git a/src/java/org/apache/cassandra/cql3/statements/SelectStatement.java b/src/java/org/apache/cassandra/cql3/statements/SelectStatement.java index 7848556e6f..21bb257714 100644 --- a/src/java/org/apache/cassandra/cql3/statements/SelectStatement.java +++ b/src/java/org/apache/cassandra/cql3/statements/SelectStatement.java @@ -178,11 +178,11 @@ public class SelectStatement implements CQLStatement { CFMetaData baseTable = View.findBaseTable(keyspace(), columnFamily()); if (baseTable != null) - state.hasColumnFamilyAccess(keyspace(), baseTable.cfName, Permission.SELECT); + state.hasColumnFamilyAccess(baseTable, Permission.SELECT); } else { - state.hasColumnFamilyAccess(keyspace(), columnFamily(), Permission.SELECT); + state.hasColumnFamilyAccess(cfm, Permission.SELECT); } for (Function function : getFunctions()) diff --git a/src/java/org/apache/cassandra/db/marshal/CompositeType.java b/src/java/org/apache/cassandra/db/marshal/CompositeType.java index 842cbbf2e9..9bd23dd7d4 100644 --- a/src/java/org/apache/cassandra/db/marshal/CompositeType.java +++ b/src/java/org/apache/cassandra/db/marshal/CompositeType.java @@ -391,7 +391,9 @@ public class CompositeType extends AbstractCompositeType for (ByteBuffer bb : buffers) { ByteBufferUtil.writeShortLength(out, bb.remaining()); - out.put(bb.duplicate()); + int toCopy = bb.remaining(); + ByteBufferUtil.arrayCopy(bb, bb.position(), out, out.position(), toCopy); + out.position(out.position() + toCopy); out.put((byte) 0); } out.flip(); diff --git a/src/java/org/apache/cassandra/db/marshal/UUIDType.java b/src/java/org/apache/cassandra/db/marshal/UUIDType.java index acaf27cb45..9722a521ea 100644 --- a/src/java/org/apache/cassandra/db/marshal/UUIDType.java +++ b/src/java/org/apache/cassandra/db/marshal/UUIDType.java @@ -140,7 +140,7 @@ public class UUIDType extends AbstractType { try { - return ByteBuffer.wrap(UUIDGen.decompose(UUID.fromString(source))); + return UUIDGen.toByteBuffer(UUID.fromString(source)); } catch (IllegalArgumentException e) { diff --git a/src/java/org/apache/cassandra/serializers/UUIDSerializer.java b/src/java/org/apache/cassandra/serializers/UUIDSerializer.java index f8e2582ec9..4501f34c77 100644 --- a/src/java/org/apache/cassandra/serializers/UUIDSerializer.java +++ b/src/java/org/apache/cassandra/serializers/UUIDSerializer.java @@ -34,7 +34,7 @@ public class UUIDSerializer implements TypeSerializer public ByteBuffer serialize(UUID value) { - return value == null ? ByteBufferUtil.EMPTY_BYTE_BUFFER : ByteBuffer.wrap(UUIDGen.decompose(value)); + return value == null ? ByteBufferUtil.EMPTY_BYTE_BUFFER : UUIDGen.toByteBuffer(value); } public void validate(ByteBuffer bytes) throws MarshalException diff --git a/src/java/org/apache/cassandra/service/ClientState.java b/src/java/org/apache/cassandra/service/ClientState.java index f2ed98477b..d576ac393f 100644 --- a/src/java/org/apache/cassandra/service/ClientState.java +++ b/src/java/org/apache/cassandra/service/ClientState.java @@ -29,6 +29,7 @@ import org.slf4j.LoggerFactory; import org.apache.cassandra.auth.*; import org.apache.cassandra.config.Config; +import org.apache.cassandra.config.CFMetaData; import org.apache.cassandra.config.DatabaseDescriptor; import org.apache.cassandra.config.Schema; import org.apache.cassandra.cql3.QueryHandler; @@ -249,6 +250,12 @@ public class ClientState hasAccess(keyspace, perm, DataResource.table(keyspace, columnFamily)); } + public void hasColumnFamilyAccess(CFMetaData cfm, Permission perm) + throws UnauthorizedException, InvalidRequestException + { + hasAccess(cfm.ksName, perm, cfm.resource); + } + private void hasAccess(String keyspace, Permission perm, DataResource resource) throws UnauthorizedException, InvalidRequestException { diff --git a/src/java/org/apache/cassandra/utils/UUIDGen.java b/src/java/org/apache/cassandra/utils/UUIDGen.java index a777a5095e..f046279053 100644 --- a/src/java/org/apache/cassandra/utils/UUIDGen.java +++ b/src/java/org/apache/cassandra/utils/UUIDGen.java @@ -106,6 +106,15 @@ public class UUIDGen return new UUID(raw.getLong(raw.position()), raw.getLong(raw.position() + 8)); } + public static ByteBuffer toByteBuffer(UUID uuid) + { + ByteBuffer buffer = ByteBuffer.allocate(16); + buffer.putLong(uuid.getMostSignificantBits()); + buffer.putLong(uuid.getLeastSignificantBits()); + buffer.flip(); + return buffer; + } + /** decomposes a uuid into raw bytes. */ public static byte[] decompose(UUID uuid) { diff --git a/test/unit/org/apache/cassandra/utils/UUIDTests.java b/test/unit/org/apache/cassandra/utils/UUIDTests.java index 83e421ad77..ebe5fd1d03 100644 --- a/test/unit/org/apache/cassandra/utils/UUIDTests.java +++ b/test/unit/org/apache/cassandra/utils/UUIDTests.java @@ -48,7 +48,6 @@ public class UUIDTests assert one.timestamp() < two.timestamp(); } - @Test public void testDecomposeAndRaw() { @@ -58,6 +57,15 @@ public class UUIDTests assert a.equals(b); } + @Test + public void testToFromByteBuffer() + { + UUID a = UUIDGen.getTimeUUID(); + ByteBuffer bb = UUIDGen.toByteBuffer(a); + UUID b = UUIDGen.getUUID(bb); + assert a.equals(b); + } + @Test public void testTimeUUIDType() {