[CASSANDRA-21536] Profile pollution in AbstractType.writeValue makes serialization slow for all column types

Description:
AbstractType.writeValue() is one shared method. All column types use it.

Inside writeValue(), it calls valueLengthIfFixed(). This is a virtual call. Many types override this method (Int32Type, LongType, UTF8Type, ...).

In a real cluster, many column types pass through writeValue(). So the profile always sees lots types.

Because of this, the JIT cannot inline the call. It stays as a vtable call. Also, the compiled body of writeValue() becomes big, so the JIT refuses to inline writeValue() itself ("already compiled into a big method").

We also see the same itable/vtable stubs in async-profiler output from a real cluster, running with default production options and a normal workload.

How to solve:
Add a final int field to AbstractType. Set it in the constructor. writeValue() reads this field instead of calling valueLengthIfFixed().

A field read needs no type profile. So profile pollution has no effect on it. The valueLengthIfFixed() method is not changed. Only the write path uses the field.

Result:
Production is always the polluted state, so this is the real-world comparison

 JMH, JDK 17 / arm64, 1 thread, 10 x 1s warmup and measurement:

                    before          after       improvement
   readValue        49.99M ops/s    56.44M ops/s  +12.90%
   writeValue       99.63M ops/s   114.46M ops/s  +14.88%

patch by Koo Taejin; reviewed by Dmitry Konstantinov, Francisco Guerrero for CASSANDRA-21536

CHANGES.txt

@@ -1,4 +1,5 @@
 6.0-alpha2
+ * Avoid megamorphic calls when serializing and deserializing fixed-length values (CASSANDRA-21536)
  * Avoid megamorphic calls for Cell.timestamp/ttl/path/localDeletionTimeAsUnsignedInt methods (CASSANDRA-21526)
  * Allow unreserved keywords as user and identity names in USER and IDENTITY statements (CASSANDRA-21510)
  * Reduce allocations in DefaultQueryOptions (CASSANDRA-21467)

src/java/org/apache/cassandra/db/marshal/AbstractTimeUUIDType.java

@@ -39,7 +39,7 @@ public abstract class AbstractTimeUUIDType<T> extends TemporalType<T>
 {
     AbstractTimeUUIDType()
     {
-        super(ComparisonType.CUSTOM);
+        super(ComparisonType.CUSTOM, 16);
     } // singleton
 
     @Override
@@ -193,12 +193,6 @@ public abstract class AbstractTimeUUIDType<T> extends TemporalType<T>
         return super.decomposeUntyped(value);
     }
 
-    @Override
-    public int valueLengthIfFixed()
-    {
-        return 16;
-    }
-
     @Override
     public long toTimeInMillis(ByteBuffer value)
     {

src/java/org/apache/cassandra/db/marshal/AbstractType.java

@@ -90,11 +90,18 @@ public abstract class AbstractType<T> implements Comparator<ByteBuffer>, Assignm
     public final ComparisonType comparisonType;
     public final boolean isByteOrderComparable;
     public final ValueComparators comparatorSet;
+    private final int valueLengthIfFixed;
 
     protected AbstractType(ComparisonType comparisonType)
+    {
+        this(comparisonType, VARIABLE_LENGTH);
+    }
+
+    protected AbstractType(ComparisonType comparisonType, int valueLengthIfFixed)
     {
         this.comparisonType = comparisonType;
         this.isByteOrderComparable = comparisonType == ComparisonType.BYTE_ORDER;
+        this.valueLengthIfFixed = valueLengthIfFixed;
         reverseComparator = (o1, o2) -> AbstractType.this.compare(o2, o1);
         try
         {
@@ -384,12 +391,12 @@ public abstract class AbstractType<T> implements Comparator<ByteBuffer>, Assignm
     /**
      * Similar to {@link #isValueCompatibleWith(AbstractType)}, but takes into account {@link Cell} encoding.
      * In particular, this method doesn't consider two types serialization compatible if one of them has fixed
-     * length (overrides {@link #valueLengthIfFixed()}, and the other one doesn't.
+     * length, and the other one doesn't.
      */
     public boolean isSerializationCompatibleWith(AbstractType<?> previous)
     {
         return isValueCompatibleWith(previous)
-               && valueLengthIfFixed() == previous.valueLengthIfFixed()
+               && valueLengthIfFixed == previous.valueLengthIfFixed
                && isMultiCell() == previous.isMultiCell();
     }
 
@@ -498,7 +505,7 @@ public abstract class AbstractType<T> implements Comparator<ByteBuffer>, Assignm
      */
     public int valueLengthIfFixed()
     {
-        return VARIABLE_LENGTH;
+        return valueLengthIfFixed;
     }
 
     /**
@@ -508,7 +515,7 @@ public abstract class AbstractType<T> implements Comparator<ByteBuffer>, Assignm
      */
     public final boolean isValueLengthFixed()
     {
-        return valueLengthIfFixed() != VARIABLE_LENGTH;
+        return valueLengthIfFixed != VARIABLE_LENGTH;
     }
 
     /**
@@ -570,7 +577,7 @@ public abstract class AbstractType<T> implements Comparator<ByteBuffer>, Assignm
     public  <V> void writeValue(V value, ValueAccessor<V> accessor, DataOutputPlus out) throws IOException
     {
         assert !isNull(value, accessor) : "bytes should not be null for type " + this;
-        int expectedValueLength = valueLengthIfFixed();
+        int expectedValueLength = valueLengthIfFixed;
         if (expectedValueLength >= 0)
         {
             int actualValueLength = accessor.size(value);
@@ -589,7 +596,7 @@ public abstract class AbstractType<T> implements Comparator<ByteBuffer>, Assignm
     public  <V> void writeValue(IndexedValueHolder<V> valueHolder, int i, ValueAccessor<V> accessor, DataOutputPlus out) throws IOException
     {
         assert !valueHolder.isNull(i) : "bytes should not be null for type " + this;
-        int expectedValueLength = valueLengthIfFixed();
+        int expectedValueLength = valueLengthIfFixed;
         if (expectedValueLength >= 0)
         {
             int actualValueLength = valueHolder.size(i);
@@ -613,7 +620,7 @@ public abstract class AbstractType<T> implements Comparator<ByteBuffer>, Assignm
     public <V> long writtenLength(V value, ValueAccessor<V> accessor)
     {
         assert !accessor.isEmpty(value) : "bytes should not be empty for type " + this;
-        return valueLengthIfFixed() >= 0
+        return valueLengthIfFixed >= 0
                ? accessor.size(value) // if the size is wrong, this will be detected in writeValue
                : accessor.sizeWithVIntLength(value);
     }
@@ -621,7 +628,7 @@ public abstract class AbstractType<T> implements Comparator<ByteBuffer>, Assignm
     public <V> long writtenLength(IndexedValueHolder<V> valueHolder, int i, ValueAccessor<V> accessor)
     {
         assert !valueHolder.isNull(i) : "bytes should not be null for type " + this;
-        return valueLengthIfFixed() >= 0
+        return valueLengthIfFixed >= 0
                ? valueHolder.size(i) // if the size is wrong, this will be detected in writeValue
                : accessor.sizeWithVIntLength(valueHolder, i);
     }
@@ -643,7 +650,7 @@ public abstract class AbstractType<T> implements Comparator<ByteBuffer>, Assignm
 
     public <V> V read(ValueAccessor<V> accessor, DataInputPlus in, int maxValueSize) throws IOException
     {
-        int length = valueLengthIfFixed();
+        int length = valueLengthIfFixed;
 
         if (length >= 0)
             return accessor.read(in, length);
@@ -664,7 +671,7 @@ public abstract class AbstractType<T> implements Comparator<ByteBuffer>, Assignm
 
     public void skipValue(DataInputPlus in) throws IOException
     {
-        int length = valueLengthIfFixed();
+        int length = valueLengthIfFixed;
         if (length >= 0)
             in.skipBytesFully(length);
         else

src/java/org/apache/cassandra/db/marshal/BooleanType.java

@@ -36,7 +36,7 @@ public class BooleanType extends AbstractType<Boolean>
     private static final ArgumentDeserializer ARGUMENT_DESERIALIZER = new DefaultArgumentDeserializer(instance);
     private static final ByteBuffer MASKED_VALUE = instance.decompose(false);
 
-    BooleanType() {super(ComparisonType.CUSTOM);} // singleton
+    BooleanType() {super(ComparisonType.CUSTOM, 1);} // singleton
 
     @Override
     public boolean allowsEmpty()
@@ -127,12 +127,6 @@ public class BooleanType extends AbstractType<Boolean>
         return ARGUMENT_DESERIALIZER;
     }
 
-    @Override
-    public int valueLengthIfFixed()
-    {
-        return 1;
-    }
-
     @Override
     public ByteBuffer getMaskedValue()
     {

src/java/org/apache/cassandra/db/marshal/DateType.java

@@ -49,7 +49,7 @@ public class DateType extends AbstractType<Date>
     private static final ArgumentDeserializer ARGUMENT_DESERIALIZER = new DefaultArgumentDeserializer(instance);
     private static final ByteBuffer MASKED_VALUE = instance.decompose(new Date(0));
 
-    DateType() {super(ComparisonType.BYTE_ORDER);} // singleton
+    DateType() {super(ComparisonType.BYTE_ORDER, 8);} // singleton
 
     public boolean isEmptyValueMeaningless()
     {
@@ -143,12 +143,6 @@ public class DateType extends AbstractType<Date>
         return ARGUMENT_DESERIALIZER;
     }
 
-    @Override
-    public int valueLengthIfFixed()
-    {
-        return 8;
-    }
-
     @Override
     public ByteBuffer getMaskedValue()
     {

src/java/org/apache/cassandra/db/marshal/DoubleType.java

@@ -40,7 +40,7 @@ public class DoubleType extends NumberType<Double>
 
     private static final ByteBuffer MASKED_VALUE = instance.decompose(0d);
 
-    DoubleType() {super(ComparisonType.CUSTOM);} // singleton
+    DoubleType() {super(ComparisonType.CUSTOM, 8);} // singleton
 
     @Override
     public boolean allowsEmpty()
@@ -145,12 +145,6 @@ public class DoubleType extends NumberType<Double>
         };
     }
 
-    @Override
-    public int valueLengthIfFixed()
-    {
-        return 8;
-    }
-
     @Override
     public ByteBuffer add(Number left, Number right)
     {

src/java/org/apache/cassandra/db/marshal/EmptyType.java

@@ -71,7 +71,7 @@ public class EmptyType extends AbstractType<Void>
 
     public static final EmptyType instance = new EmptyType();
 
-    private EmptyType() {super(ComparisonType.CUSTOM);} // singleton
+    private EmptyType() {super(ComparisonType.CUSTOM, 0);} // singleton
 
     @Override
     public <V> ByteSource asComparableBytes(ValueAccessor<V> accessor, V data, ByteComparable.Version version)
@@ -137,12 +137,6 @@ public class EmptyType extends AbstractType<Void>
         throw new UnsupportedOperationException();
     }
 
-    @Override
-    public int valueLengthIfFixed()
-    {
-        return 0;
-    }
-
     @Override
     public <V> long writtenLength(V value, ValueAccessor<V> accessor)
     {

src/java/org/apache/cassandra/db/marshal/FloatType.java

@@ -41,7 +41,7 @@ public class FloatType extends NumberType<Float>
 
     private static final ByteBuffer MASKED_VALUE = instance.decompose(0f);
 
-    FloatType() {super(ComparisonType.CUSTOM);} // singleton
+    FloatType() {super(ComparisonType.CUSTOM, 4);} // singleton
 
     @Override
     public boolean allowsEmpty()
@@ -146,12 +146,6 @@ public class FloatType extends NumberType<Float>
         };
     }
 
-    @Override
-    public int valueLengthIfFixed()
-    {
-        return 4;
-    }
-
     @Override
     public ByteBuffer add(Number left, Number right)
     {

src/java/org/apache/cassandra/db/marshal/Int32Type.java

@@ -43,7 +43,7 @@ public class Int32Type extends NumberType<Integer>
 
     Int32Type()
     {
-        super(ComparisonType.CUSTOM);
+        super(ComparisonType.CUSTOM, 4);
     } // singleton
 
     @Override
@@ -152,12 +152,6 @@ public class Int32Type extends NumberType<Integer>
         };
     }
 
-    @Override
-    public int valueLengthIfFixed()
-    {
-        return 4;
-    }
-
     @Override
     public ByteBuffer add(Number left, Number right)
     {

src/java/org/apache/cassandra/db/marshal/LexicalUUIDType.java

@@ -44,7 +44,7 @@ public class LexicalUUIDType extends AbstractType<UUID>
 
     LexicalUUIDType()
     {
-        super(ComparisonType.CUSTOM);
+        super(ComparisonType.CUSTOM, 16);
     } // singleton
 
     @Override
@@ -148,12 +148,6 @@ public class LexicalUUIDType extends AbstractType<UUID>
         return ARGUMENT_DESERIALIZER;
     }
 
-    @Override
-    public int valueLengthIfFixed()
-    {
-        return 16;
-    }
-
     @Override
     public ByteBuffer getMaskedValue()
     {

src/java/org/apache/cassandra/db/marshal/LongType.java

@@ -41,7 +41,7 @@ public class LongType extends NumberType<Long>
 
     private static final ByteBuffer MASKED_VALUE = instance.decompose(0L);
 
-    LongType() {super(ComparisonType.CUSTOM);} // singleton
+    LongType() {super(ComparisonType.CUSTOM, 8);} // singleton
 
     @Override
     public boolean allowsEmpty()
@@ -170,12 +170,6 @@ public class LongType extends NumberType<Long>
         };
     }
 
-    @Override
-    public int valueLengthIfFixed()
-    {
-        return 8;
-    }
-
     @Override
     public ByteBuffer add(Number left, Number right)
     {

src/java/org/apache/cassandra/db/marshal/MultiElementType.java

@@ -38,6 +38,11 @@ public abstract class MultiElementType<T> extends AbstractType<T>
         super(comparisonType);
     }
 
+    protected MultiElementType(ComparisonType comparisonType, int valueLengthIfFixed)
+    {
+        super(comparisonType, valueLengthIfFixed);
+    }
+
     /**
      * Returns the serialized representation of the value composed of the specified elements.
      *
@@ -133,4 +138,3 @@ public abstract class MultiElementType<T> extends AbstractType<T>
         throw new UnsupportedOperationException(this + " does not support retrieving elements by key or index");
     }
 }
-

src/java/org/apache/cassandra/db/marshal/NumberType.java

@@ -34,6 +34,11 @@ public abstract class NumberType<T extends Number> extends AbstractType<T>
         super(comparisonType);
     }
 
+    protected NumberType(ComparisonType comparisonType, int valueLengthIfFixed)
+    {
+        super(comparisonType, valueLengthIfFixed);
+    }
+
     /**
      * Checks if this type support floating point numbers.
      * @return {@code true} if this type support floating point numbers, {@code false} otherwise.

src/java/org/apache/cassandra/db/marshal/ReversedType.java

@@ -57,7 +57,7 @@ public class ReversedType<T> extends AbstractType<T>
 
     private ReversedType(AbstractType<T> baseType)
     {
-        super(ComparisonType.CUSTOM);
+        super(ComparisonType.CUSTOM, baseType.valueLengthIfFixed());
         this.baseType = baseType;
     }
 
@@ -181,12 +181,6 @@ public class ReversedType<T> extends AbstractType<T>
         return getInstance(baseType.withUpdatedUserType(udt));
     }
 
-    @Override
-    public int valueLengthIfFixed()
-    {
-        return baseType.valueLengthIfFixed();
-    }
-
     @Override
     public boolean isReversed()
     {

src/java/org/apache/cassandra/db/marshal/TemporalType.java

@@ -37,6 +37,11 @@ public abstract class TemporalType<T> extends AbstractType<T>
         super(comparisonType);
     }
 
+    protected TemporalType(ComparisonType comparisonType, int valueLengthIfFixed)
+    {
+        super(comparisonType, valueLengthIfFixed);
+    }
+
     /**
      * Returns the current temporal value.
      * @return the current temporal value.

src/java/org/apache/cassandra/db/marshal/TimestampType.java

@@ -53,7 +53,7 @@ public class TimestampType extends TemporalType<Date>
 
     private static final ByteBuffer MASKED_VALUE = instance.decompose(new Date(0));
 
-    private TimestampType() {super(ComparisonType.CUSTOM);} // singleton
+    private TimestampType() {super(ComparisonType.CUSTOM, 8);} // singleton
 
     @Override
     public boolean allowsEmpty()
@@ -166,12 +166,6 @@ public class TimestampType extends TemporalType<Date>
         return TimestampSerializer.instance;
     }
 
-    @Override
-    public int valueLengthIfFixed()
-    {
-        return 8;
-    }
-
     @Override
     protected void validateDuration(Duration duration)
     {

src/java/org/apache/cassandra/db/marshal/UUIDType.java

@@ -56,7 +56,7 @@ public class UUIDType extends AbstractType<UUID>
 
     UUIDType()
     {
-        super(ComparisonType.CUSTOM);
+        super(ComparisonType.CUSTOM, 16);
     }
 
     @Override
@@ -252,12 +252,6 @@ public class UUIDType extends AbstractType<UUID>
         return (uuid.get(6) & 0xf0) >> 4;
     }
 
-    @Override
-    public int valueLengthIfFixed()
-    {
-        return 16;
-    }
-
     @Override
     public ByteBuffer getMaskedValue()
     {

src/java/org/apache/cassandra/db/marshal/VectorType.java

@@ -82,20 +82,16 @@ public final class VectorType<T> extends MultiElementType<List<T>>
     public final AbstractType<T> elementType;
     public final int dimension;
     private final TypeSerializer<T> elementSerializer;
-    private final int valueLengthIfFixed;
     private final VectorSerializer serializer;
 
     private VectorType(AbstractType<T> elementType, int dimension)
     {
-        super(ComparisonType.CUSTOM);
+        super(ComparisonType.CUSTOM, valueLengthIfFixed(elementType, dimension));
         if (dimension <= 0)
             throw new InvalidRequestException(String.format("vectors may only have positive dimensions; given %d", dimension));
         this.elementType = elementType;
         this.dimension = dimension;
         this.elementSerializer = elementType.getSerializer();
-        this.valueLengthIfFixed = elementType.isValueLengthFixed() ?
-                                  elementType.valueLengthIfFixed() * dimension :
-                                  super.valueLengthIfFixed();
         this.serializer = elementType.isValueLengthFixed() ?
                           new FixedLengthSerializer() :
                           new VariableLengthSerializer();
@@ -126,10 +122,10 @@ public final class VectorType<T> extends MultiElementType<List<T>>
         return getSerializer().compareCustom(left, accessorL, right, accessorR);
     }
 
-    @Override
+    private static int valueLengthIfFixed(AbstractType<?> elementType, int dimension)
-    public int valueLengthIfFixed()
     {
-        return valueLengthIfFixed;
+        int elementLength = elementType.valueLengthIfFixed();
+        return elementLength >= 0 ? elementLength * dimension : elementLength;
     }
 
     @Override

test/microbench/org/apache/cassandra/test/microbench/AbstractTypeSerializationBench.java

@@ -0,0 +1,94 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.test.microbench;
+
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.util.concurrent.TimeUnit;
+
+import org.openjdk.jmh.annotations.Benchmark;
+import org.openjdk.jmh.annotations.BenchmarkMode;
+import org.openjdk.jmh.annotations.Fork;
+import org.openjdk.jmh.annotations.Measurement;
+import org.openjdk.jmh.annotations.Mode;
+import org.openjdk.jmh.annotations.OutputTimeUnit;
+import org.openjdk.jmh.annotations.Scope;
+import org.openjdk.jmh.annotations.Setup;
+import org.openjdk.jmh.annotations.State;
+import org.openjdk.jmh.annotations.Threads;
+import org.openjdk.jmh.annotations.Warmup;
+
+import org.apache.cassandra.db.marshal.AbstractType;
+import org.apache.cassandra.db.marshal.BooleanType;
+import org.apache.cassandra.db.marshal.Int32Type;
+import org.apache.cassandra.db.marshal.LongType;
+import org.apache.cassandra.db.marshal.UTF8Type;
+import org.apache.cassandra.db.marshal.UUIDType;
+import org.apache.cassandra.io.util.DataInputBuffer;
+import org.apache.cassandra.io.util.DataOutputBuffer;
+import org.apache.cassandra.utils.ByteBufferUtil;
+
+/**
+ * Measures value serialization when one call site sees the fixed- and variable-width types commonly used by a table.
+ */
+@BenchmarkMode(Mode.Throughput)
+@OutputTimeUnit(TimeUnit.SECONDS)
+@Warmup(iterations = 5, time = 1)
+@Measurement(iterations = 5, time = 1)
+@Fork(value = 1, jvmArgsAppend = { "-Xmx1G", "-Djmh.executor=CUSTOM", "-Djmh.executor.class=org.apache.cassandra.test.microbench.FastThreadExecutor" })
+@Threads(1)
+@State(Scope.Thread)
+public class AbstractTypeSerializationBench
+{
+    private final AbstractType<?>[] types = { Int32Type.instance, LongType.instance, BooleanType.instance, UUIDType.instance,
+                                              UTF8Type.instance, Int32Type.instance, LongType.instance, UTF8Type.instance };
+    private final ByteBuffer[] values = { ByteBufferUtil.bytes(42), ByteBufferUtil.bytes(42L), ByteBuffer.wrap(new byte[] { 1 }),
+                                          ByteBuffer.wrap(new byte[16]), ByteBufferUtil.bytes("cassandra"), ByteBufferUtil.bytes(42),
+                                          ByteBufferUtil.bytes(42L), ByteBufferUtil.bytes("cassandra") };
+    private final ByteBuffer[] serializedValues = new ByteBuffer[types.length];
+    private final DataOutputBuffer out = new DataOutputBuffer();
+    private int index;
+
+    @Setup
+    public void setup() throws IOException
+    {
+        for (int i = 0; i < types.length; i++)
+        {
+            out.clear();
+            types[i].writeValue(values[i], out);
+            serializedValues[i] = out.asNewBuffer();
+        }
+    }
+
+    @Benchmark
+    public int writeValue() throws IOException
+    {
+        int i = index++ & (types.length - 1);
+        out.clear();
+        types[i].writeValue(values[i], out);
+        return out.getLength();
+    }
+
+    @Benchmark
+    public ByteBuffer readValue() throws IOException
+    {
+        int i = index++ & (types.length - 1);
+        return types[i].readBuffer(new DataInputBuffer(serializedValues[i], true));
+    }
+}

test/unit/org/apache/cassandra/db/marshal/AbstractTypeTest.java

@@ -739,6 +739,12 @@ public class AbstractTypeTest
             }});
     }
 
+    @Test
+    public void valueLengthIfFixedIsNotFinal() throws NoSuchMethodException
+    {
+        assertThat(Modifier.isFinal(AbstractType.class.getMethod("valueLengthIfFixed").getModifiers())).isFalse();
+    }
+
     @Test
     @SuppressWarnings({"rawtypes", "unchecked"})
     public void serde()