[jira] [Assigned] (CASSANDRA-21587) Megamorphic call in ClusteringPrefix.Deserializer.deserializeOne slows down reads

["Dmitry Konstantinov (Jira)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/CASSANDRA-21587?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Dmitry Konstantinov reassigned CASSANDRA-21587:
-----------------------------------------------

    Assignee: Dmitry Konstantinov

> Megamorphic call in ClusteringPrefix.Deserializer.deserializeOne slows down reads
> ---------------------------------------------------------------------------------
>
>                 Key: CASSANDRA-21587
>                 URL: https://issues.apache.org/jira/browse/CASSANDRA-21587
>             Project: Apache Cassandra
>          Issue Type: Improvement
>            Reporter: koo
>            Assignee: Dmitry Konstantinov
>            Priority: Normal
>
> h2. Problem
> {{ClusteringPrefix.Deserializer.deserializeOne()}} reads one clustering component at a
> time. It is the inner loop of a partition scan, so it runs very often. It has this line
> (ClusteringPrefix.java:674 on cassandra-5.0, :725 on trunk):
> {code:java}
> serializationHeader.clusteringTypes().get(i).readArray(in, DatabaseDescriptor.getMaxValueSize())
> {code}
> {{clusteringTypes()}} returns a {{{}java.util.List{}}}, so {{get}} is an interface call.
> Three classes can arrive here:
> ||Where the header comes from||Class of the list||
> |sstable Stats component read from disk|{{java.util.ArrayList}}|
> |flush or compaction, 1 clustering column|{{SingletonImmutableList}}|
> |flush or compaction, 2 or more clustering columns|{{RegularImmutableList}}|
> The first comes from {{AbstractTypeSerializer.deserializeList()}}
> ({{{}new ArrayList<>(size){}}}). The other two come from {{ClusteringComparator.subtypes()}}
> ({{{}ImmutableList.copyOf(...){}}}), which returns a different class for size 1 and size 2+.
> HotSpot can inline one or two receiver classes. With three it gives up and uses an itable
> stub, which costs much more than the work itself. {{get}} is only one array load.
> {{ClusteringComparator}} already has the comment
> {{{}// copy the list to ensure despatch is monomorphic{}}}, but it does not hold today.
> h3. Numbers
> Production node, async-profiler, 120s, 97,036 CPU samples:
> |{{deserializeOne}} (inclusive)|4,869|5.0% of all CPU|
> |itable stub from that one line|608|0.63% of all CPU|
> About {*}1/8 of the CPU spent in {{deserializeOne}} is only dispatch{*}, not real work.
> JMH benchmark with the receiver count controlled:
> |3 classes|1129.1 +- 17.1 ns/op|
> |2 classes|981.4 +- 34.8 ns/op|
> |1 class|934.7 +- 27.4 ns/op|
> Three to two is {*}13.1% faster{*}, and the intervals do not overlap.
> h2. Fix
> *Option 1 - one file, four lines.* Make {{AbstractTypeSerializer.deserializeList()}} return
> an immutable list. The return type stays {{{}List{}}}, so nothing else changes.
> {code:java}
> -    List<AbstractType<?>> types = new ArrayList<>(size);
> +    ImmutableList.Builder<AbstractType<?>> types = ImmutableList.builderWithExpectedSize(size);
>      for (int i = 0; i < size; i++)
>          types.add(deserialize(in));
> -    return types;
> +    return types.build();
> {code}
> *Option 2 - type the chain as {{ImmutableList}}* ({{{}ClusteringComparator{}}},
> {{{}SerializationHeader{}}}, the clustering serializers, {{{}Slice{}}}, {{{}Slices{}}},
> {{{}IndexInfo{}}}, {{{}StatsMetadata{}}}; about 12 files, all mechanical).  
> It could stop the same mistake from coming back.
> We suggest Option 1 first, and Option 2 as a follow-up if people want the type safety.
> h2. Notes
>  * We see this on a cluster we run in production on 5.0.8, not only in the profile.
>  * The same code is on trunk; only line numbers differ.



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org