[
https://issues.apache.org/jira/browse/CASSANDRA-21535?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18110135#comment-18110135
]
koo commented on CASSANDRA-21535:
---------------------------------
While looking into the two CompactionsCQLTest failures
(testCompactionInvalidRowDeletion and testIndexedReaderRowDeletion), I noticed
that EncodingStats.Serializer computes the size in a different way from how it
writes:
public void serialize(EncodingStats stats, DataOutputPlus out) throws
IOException
\{
out.writeUnsignedVInt(stats.minTimestamp - TIMESTAMP_EPOCH);
out.writeUnsignedVInt32((int)(stats.minLocalDeletionTime -
DELETION_TIME_EPOCH));
out.writeUnsignedVInt32(stats.minTTL - TTL_EPOCH);
}
public int serializedSize(EncodingStats stats)
\{
return
TypeSizes.sizeofUnsignedVInt(stats.minTimestamp - TIMESTAMP_EPOCH)
+ TypeSizes.sizeofUnsignedVInt(stats.minLocalDeletionTime -
DELETION_TIME_EPOCH)
+ TypeSizes.sizeofUnsignedVInt(stats.minTTL - TTL_EPOCH);
}
The write path narrows the value to an int first. The size path keeps it as a
long. For values that fit in a signed int the two agree, so this has not been
visible until now.
It becomes visible when minLocalDeletionTime holds an invalid or capped
deletion time. In that case the difference from DELETION_TIME_EPOCH is around
2.85e9, which does not fit in a signed int:
- size path: sizeofUnsignedVInt(2852087294L) → 5 bytes
- write path: the (int) cast wraps to -1442880002, and writeUnsignedVInt32
sign-extends it back to a long → 9 bytes
Before this patch the growable DataOutputBuffer just absorbed the extra 4
bytes, so nothing failed. With the new DataOutputBufferFixed sized from
serializedSize(), it now fails with BufferOverflowException.
Making the two methods agree looks simple. Applying the same (int) cast on the
size side is enough to make both tests pass, and it does not change any written
byte. SerializationHeader.writeLocalDeletionTime() and
localDeletionTimeSerializedSize() have the same shape, so they would need the
same treatment.
That said, I think there may be a more fundamental issue underneath. The
written value itself does not round-trip: deserialize() reads it back with
readUnsignedVInt32(), so minLocalDeletionTime returns as -2 instead of the
original value. Since it is the baseline for the delta-encoded local deletion
times, every value derived from it would be off as well.
Fixing that would mean changing what is written, and this serializer is used
for SSTables as well as messaging. So it would need version gating and a
decision about already written data. That feels outside the scope of this
ticket.
> Avoid extra copy for cached Mutation serialization
> --------------------------------------------------
>
> Key: CASSANDRA-21535
> URL: https://issues.apache.org/jira/browse/CASSANDRA-21535
> Project: Apache Cassandra
> Issue Type: Improvement
> Components: Local/Other
> Reporter: koo
> Assignee: koo
> Priority: Normal
> Fix For: 6.0.x, 7.x
>
> Attachments: CASSANDRA-21535-trunk_ci_summary.htm,
> CASSANDRA-21535-trunk_results_details.tar.xz, profiler-after.zip,
> profiler-before.zip
>
> Time Spent: 2h
> Remaining Estimate: 0h
>
> Description
> In Mutation.Serializer.serialization(), we first calculate the serialized
> size.
> After that, current code serializes the mutation to a thread local
> DataOutputBuffer.
> Then unsafeToByteArray() creates a new byte array and copies all serialized
> bytes.
> The serialized size is already known.
> So we can create a fixed size heap buffer first and serialize directly to
> that buffer.
> This change removes one full byte array copy when we create a cached mutation
> serialization.
> I also added a check that the written size is same as the calculated
> serialized size.
> Benchmark
> I added a JMH benchmark for this case.
> For every benchmark operation:
> - clear cached serialization
> - serialize the Mutation
> - create cached serialization again
> JDK 17, arm64, 1 thread:
> | Value size | Before | After | Result |
> |---:|---:|---:|---:|
> | 128 B | 3.70 M ops/s | 3.87 M ops/s | +4% |
> | 16 KiB | 608 K ops/s | 716 K ops/s | +18% |
> | 256 KiB | 41.4 K ops/s | 53.1 K ops/s | +28% |
> | 768 KiB | 6.83 K ops/s | 18.88 K ops/s | +177% |
>
> The result is bigger for large mutations because current code writes the data
> to a
> direct buffer first, and then copies all data again to a heap byte array.
> With this change, serialization writes directly to the final heap byte array.
> This benchmark measures cache creation cost. It does not mean normal write
> throughput will be 2.77x faster, because normal requests can reuse the cached
> serialization.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
