org.apache.cassandra.db.rows.ArrayCell#unsharedHeapSizeExcludingData …

…includes data twice

patch by David Capwell; reviewed by Caleb Rackliffe for CASSANDRA-16900

CHANGES.txt

@@ -2,6 +2,7 @@
  * Fix clustering order logic in CREATE MATERIALIZED VIEW (CASSANDRA-16898)
 
 4.0.1
+ * org.apache.cassandra.db.rows.ArrayCell#unsharedHeapSizeExcludingData includes data twice (CASSANDRA-16900)
  * Exclude Jackson 1.x transitive dependency of hadoop* provided dependencies (CASSANDRA-16854)
  * Tolerate missing DNS entry when completing a host replacement (CASSANDRA-16873)
  * Harden PrunableArrayQueue against Pruner implementations that might throw exceptions (CASSANDRA-16866)

src/java/org/apache/cassandra/cql3/ColumnIdentifier.java

@@ -205,7 +205,7 @@ public long unsharedHeapSize()
     public long unsharedHeapSizeExcludingData()
     {
         return EMPTY_SIZE
-             + ObjectSizes.sizeOnHeapExcludingData(bytes)
+             + ObjectSizes.sizeOfEmptyHeapByteBuffer()
              + ObjectSizes.sizeOf(text);
     }
 

src/java/org/apache/cassandra/db/rows/ArrayCell.java

@@ -112,6 +112,6 @@ public Cell<?> copy(AbstractAllocator allocator)
 
     public long unsharedHeapSizeExcludingData()
     {
-        return EMPTY_SIZE + ObjectSizes.sizeOfArray(value) + value.length + (path == null ? 0 : path.unsharedHeapSizeExcludingData());
+        return EMPTY_SIZE + ObjectSizes.sizeOfEmptyByteArray() + (path == null ? 0 : path.unsharedHeapSizeExcludingData());
     }
 }

src/java/org/apache/cassandra/db/rows/BufferCell.java

@@ -28,6 +28,8 @@
 import org.apache.cassandra.utils.ObjectSizes;
 import org.apache.cassandra.utils.memory.AbstractAllocator;
 
+import static java.lang.String.format;
+
 public class BufferCell extends AbstractCell<ByteBuffer>
 {
     private static final long EMPTY_SIZE = ObjectSizes.measure(new BufferCell(ColumnMetadata.regularColumn("", "", "", ByteType.instance), 0L, 0, 0, ByteBufferUtil.EMPTY_BYTE_BUFFER, null));
@@ -43,7 +45,7 @@ public BufferCell(ColumnMetadata column, long timestamp, int ttl, int localDelet
     {
         super(column);
         assert !column.isPrimaryKeyColumn();
-        assert column.isComplex() == (path != null);
+        assert column.isComplex() == (path != null) : format("Column %s.%s(%s: %s) isComplex: %b with cellpath: %s", column.ksName, column.cfName, column.name, column.type.toString(), column.isComplex(), path);
         this.timestamp = timestamp;
         this.ttl = ttl;
         this.localDeletionTime = localDeletionTime;
@@ -142,6 +144,6 @@ public Cell<?> copy(AbstractAllocator allocator)
 
     public long unsharedHeapSizeExcludingData()
     {
-        return EMPTY_SIZE + ObjectSizes.sizeOnHeapExcludingData(value) + (path == null ? 0 : path.unsharedHeapSizeExcludingData());
+        return EMPTY_SIZE + ObjectSizes.sizeOfEmptyHeapByteBuffer() + (path == null ? 0 : path.unsharedHeapSizeExcludingData());
     }
 }

src/java/org/apache/cassandra/db/rows/CellPath.java

@@ -127,7 +127,7 @@ public CellPath copy(AbstractAllocator allocator)
 
         public long unsharedHeapSizeExcludingData()
         {
-            return EMPTY_SIZE + ObjectSizes.sizeOnHeapExcludingData(value);
+            return EMPTY_SIZE + ObjectSizes.sizeOfEmptyHeapByteBuffer();
         }
     }
 

src/java/org/apache/cassandra/db/rows/NativeCell.java

@@ -78,7 +78,7 @@ public NativeCell(NativeAllocator allocator,
         assert column.isComplex() == (path != null);
         if (path != null)
         {
-            assert path.size() == 1;
+            assert path.size() == 1 : String.format("Expected path size to be 1 but was not; %s", path);
             size += 4 + path.get(0).remaining();
         }
 

src/java/org/apache/cassandra/fql/FullQueryLogger.java

@@ -80,7 +80,7 @@ public class FullQueryLogger implements QueryEvents.Listener
     public static final String QUERIES = "queries";
     public static final String VALUES = "values";
 
-    private static final int EMPTY_BYTEBUFFER_SIZE = Ints.checkedCast(ObjectSizes.sizeOnHeapExcludingData(ByteBuffer.allocate(0)));
+    private static final int EMPTY_BYTEBUFFER_SIZE = Ints.checkedCast(ObjectSizes.sizeOfEmptyHeapByteBuffer());
 
     private static final int EMPTY_LIST_SIZE = Ints.checkedCast(ObjectSizes.measureDeep(new ArrayList(0)));
     private static final int EMPTY_BYTEBUF_SIZE;

src/java/org/apache/cassandra/utils/ObjectSizes.java

@@ -37,6 +37,7 @@ public class ObjectSizes
                                              .ignoreKnownSingletons();
 
     private static final long BUFFER_EMPTY_SIZE = measure(ByteBufferUtil.EMPTY_BYTE_BUFFER);
+    private static final long BYTE_ARRAY_EMPTY_SIZE = measure(new byte[0]);
     private static final long STRING_EMPTY_SIZE = measure("");
 
     /**
@@ -128,11 +129,16 @@ public static long sizeOnHeapOf(ByteBuffer buffer)
         return BUFFER_EMPTY_SIZE + sizeOfArray(buffer.capacity(), 1);
     }
 
-    public static long sizeOnHeapExcludingData(ByteBuffer buffer)
+    public static long sizeOfEmptyHeapByteBuffer()
     {
         return BUFFER_EMPTY_SIZE;
     }
 
+    public static long sizeOfEmptyByteArray()
+    {
+        return BYTE_ARRAY_EMPTY_SIZE;
+    }
+
     /**
      * Memory a String consumes
      * @param str String to calculate memory size of

test/unit/org/apache/cassandra/db/CellSpecTest.java

@@ -0,0 +1,121 @@
+/*
+ * 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.db;
+
+import java.nio.ByteBuffer;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+import java.util.concurrent.CompletableFuture;
+import java.util.function.BiConsumer;
+import java.util.stream.Collectors;
+
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+import org.apache.cassandra.db.marshal.BytesType;
+import org.apache.cassandra.db.marshal.ListType;
+import org.apache.cassandra.db.rows.ArrayCell;
+import org.apache.cassandra.db.rows.BufferCell;
+import org.apache.cassandra.db.rows.Cell;
+import org.apache.cassandra.db.rows.CellPath;
+import org.apache.cassandra.db.rows.NativeCell;
+import org.apache.cassandra.schema.ColumnMetadata;
+import org.apache.cassandra.schema.TableMetadata;
+import org.apache.cassandra.utils.ObjectSizes;
+import org.apache.cassandra.utils.UUIDGen;
+import org.apache.cassandra.utils.concurrent.OpOrder;
+import org.apache.cassandra.utils.memory.NativeAllocator;
+import org.apache.cassandra.utils.memory.NativePool;
+import org.assertj.core.api.Assertions;
+
+import static org.apache.cassandra.utils.ByteBufferUtil.bytes;
+
+@RunWith(Parameterized.class)
+public class CellSpecTest
+{
+    private final Cell<?> cell;
+
+    @SuppressWarnings("unused")
+    public CellSpecTest(String ignoreOnlyUsedForBetterTestName, Cell<?> cell)
+    {
+        this.cell = cell;
+    }
+
+    @Test
+    public void unsharedHeapSizeExcludingData()
+    {
+        long empty = ObjectSizes.measure(cell);
+        long expected;
+        if (cell instanceof NativeCell)
+        {
+            // NativeCell stores the contents off-heap, so the cost on-heap is just the object's empty case
+            expected = empty;
+        }
+        else
+        {
+            // size should be: empty + valuePtr + path.unsharedHeapSizeExcludingData() if present
+            expected = empty + valuePtrSize(cell.value());
+            if (cell.path() != null)
+                expected += cell.path().unsharedHeapSizeExcludingData();
+        }
+
+        Assertions.assertThat(cell.unsharedHeapSizeExcludingData())
+                  .isEqualTo(expected);
+    }
+
+    private static long valuePtrSize(Object value)
+    {
+        if (value instanceof ByteBuffer)
+            return ObjectSizes.sizeOfEmptyHeapByteBuffer();
+        else if (value instanceof byte[])
+            return ObjectSizes.sizeOfEmptyByteArray();
+        throw new IllegalArgumentException("Unsupported type: " + value.getClass());
+    }
+
+    @Parameterized.Parameters(name = "{0}")
+    public static Collection<Object[]> data() {
+        TableMetadata table = TableMetadata.builder("testing", "testing")
+                                           .addPartitionKeyColumn("pk", BytesType.instance)
+                                           .build();
+
+        byte[] rawBytes = { 0, 1, 2, 3, 4, 5, 6 };
+        ByteBuffer bbBytes = ByteBuffer.wrap(rawBytes);
+        NativePool pool = new NativePool(1024, 1024, 1, () -> CompletableFuture.completedFuture(true));
+        NativeAllocator allocator = pool.newAllocator();
+        OpOrder order = new OpOrder();
+
+        List<Cell<?>> tests = new ArrayList<>();
+        BiConsumer<ColumnMetadata, CellPath> fn = (column, path) -> {
+            tests.add(new ArrayCell(column, 1234, 1, 1, rawBytes, path));
+            tests.add(new BufferCell(column, 1234, 1, 1, bbBytes, path));
+            tests.add(new NativeCell(allocator, order.getCurrent(), column, 1234, 1, 1, bbBytes, path));
+        };
+        // simple
+        fn.accept(ColumnMetadata.regularColumn(table, bytes("simple"), BytesType.instance), null);
+
+        // complex
+        // seems NativeCell does not allow CellPath.TOP, or CellPath.BOTTOM
+        fn.accept(ColumnMetadata.regularColumn(table, bytes("complex"), ListType.getInstance(BytesType.instance, true)), CellPath.create(bytes(UUIDGen.getTimeUUID())));
+
+        return tests.stream().map(a -> new Object[] {a.getClass().getSimpleName() + ":" + (a.path() == null ? "simple" : "complex"), a}).collect(Collectors.toList());
+    }
+
+}