[
https://issues.apache.org/jira/browse/CASSANDRA-20167?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17912258#comment-17912258
]
Dmitry Konstantinov edited comment on CASSANDRA-20167 at 1/12/25 1:02 PM:
--------------------------------------------------------------------------
Hi, I have decomposed the whole change set to the following logical parts:
||N||Change scope|| Alloc benefit as % of the full change set||API
changes||MR/patch||Suggestion||
|1|Avoid unnecessary iterators for the write hot path, do not recreate
immutable objects with the same content, avoid objects allocations used only in
rarely invoked logic branches, memorize things which can be computed
once|60%|single primitive one: to add an override method
VIewManager.updatesAffectView with a single element to avoid collection
creation to invoke it|TBD|to merge to 5.0 in this ticket|
|2|Reduce memory allocation in CQL transport logic:
* Parse header and query options flags as int, without EnumSet overheads
* avoid ByteBuffer slicing in 2 places
* avoid a capturing lambda allocation by using an explicit
interface|5.5%|Small API changes limited to interaction between few classes: *
expose Header flags as int instead of EnumSet - it impacts only 2 classes
within the same package
* add an explicit functional interface for OnFlushCleanup instead of
Consumer<FlushItem<Envelope>>|[^20167_network-5.0.patch]|to merge to 5.0 in
this ticket if possible or to extract to a separate ticket to evaluate
separately|
|3|Reduce memory allocation Mutation logic:
* avoid an unnecessary byte buffer slicing (DataOutputBuffer.toByteArray())
during a Mutation serialization
* Replace AtomicLong viewLockAcquireStart field in Mutation with volatile
long and AtomicLongFieldUpdater usage|5%|Add new method to
DataOutputBuffer.unsafeToByteArray() (similar to a recently added
unsafeGetBufferAndFlip())|[^20167_mutation-5.0.patch]|to merge to 5.0 in this
ticket if possible or to extract to a separate ticket to evaluate separately|
|4|Use simplified iterator logic for PartitionUpdate to avoid
LeafBTreeSearchIterator within inclusionTester() logic which checks if we need
to iterate over particular column for each cell. It is not really needed here:
we've just added these cells during PartitionUpdate creation|7.6%|Add new
builder method in ColumnFilter: all(RegularAndStaticColumns
columns)|[^20167_partition_update_iterator-5.0.patch]|to merge to 5.0 in this
ticket if possible or to extract to a separate ticket to evaluate separately|
|5|Cache BtreeRow.builder in a thread-local tiny pool to reuse in
UpdateParameters|4.6%|BTreeRow.pooledUnsortedBuilder method is
added|[^20167_BTreeRow_Builder_reuse-5.0.patch]|to merge to 5.0 in this ticket
if possible or to extract to a separate ticket to evaluate separately|
|6|Add optimized version of UpdatesCollector for a single partition update
within a single table (a very typical case when we just modify a row)|10%|No
existing API changes.
We introduce a new implementation for UpdatesCollector, theoretically we have
now 3 implementations of UpdatesCollector interface and
addUpdates() call can be megamorphic but it is not an issue, because:
1) multi-table batch is no a frequent case (so, in majority of cases the call
will be bimorphic)
2) addUpdates is big enough by itself and invoked once per partition, so the
overheads of a megamorphic call for this callsite (even if we have it) are
limited
3) benefits to not create and fill HashMultiset + 2 HashMap instances are much
higher |[^20167_SingleTableSinglePartitionUpdatesCollector-5.0.patch]|to merge
to 5.0 in this ticket if possible or to extract to a separate ticket to
evaluate separately|
|7|Optimize a typical case: a single clustering key in an update|3%|Add new
method to
MultiCBuilder.buildSingle()|[^20167_single_clustering_key-5.0.patch]|Can be
moved to 5.1 if needed|
|8|Avoid ImmutableList.copyOf(columnSpecs) within
OptionsWithColumnSpecifications constructor, we have to copy columnSpecs for
every request because in one place plain List is used and in another -
ImmutableList|2.3%|Change collection type from List to ImmutableList for a set
of methods in CQLStatement/QueryOptionsAdd a new method to
VariableSpecifications. A simple change but affects many
classes.|[^20167_bind_vars_immutable_list-5.0.patch]|Move to 5.1|
|9|iterate over restriction set as an array to avoid iterator usage|0.1%|Add
RestrictionSet.toArray()
method|[^20167_restriction_set_as_array-5.0.patch]|Move to 5.1 or drop due to a
too small win|
note: I rounded % so the sum is slightly less than 100%
So, the suggestion:
# To merge to 5.0 in the context of the current story, options:
## minimal (1) row from the table; 60% of benefits, basic changes
## medium: (1) + (4) + (6); 77.6% of benefits
## bigger: (1) + (2) + (3) + (4) + (5) + (6) + (7)?; 95.7% of benefits
# (9) change I suggest to just drop due to a too small win by itself for the
amount of changes
# (8) or (7) + (8) - move to 5.1
# if something from (2) + (3) + (4) + (5) + (6) + (7) is too big in
combination with others for this story - we can evaluate them more precisely
part by part in separate stories (each change by itself is quite simple and
straightforward).
was (Author: dnk):
Hi, I have decomposed the whole change set to the following logical parts:
||N||Change scope|| Alloc benefit as % of the full change set||API
changes||MR/patch||Suggestion||
|1|Avoid unnecessary iterators for the write hot path, do not recreate
immutable objects with the same content, avoid objects allocations used only in
rarely invoked logic branches, memorize things which can be computed
once|60%|single primitive one: to add an override method
VIewManager.updatesAffectView with a single element to avoid collection
creation to invoke it|TBD|to merge to 5.0 in this ticket|
|2|Reduce memory allocation in CQL transport logic:
* Parse header and query options flags as int, without EnumSet overheads
* avoid ByteBuffer slicing in 2 places
* avoid a capturing lambda allocation by using an explicit
interface|5.5%|Small API changes limited to interaction between few classes: *
expose Header flags as int instead of EnumSet - it impacts only 2 classes
within the same package
* add an explicit functional interface for OnFlushCleanup instead of
Consumer<FlushItem<Envelope>>|TBD|to merge to 5.0 in this ticket if possible or
to extract to a separate ticket to evaluate separately|
|3|Reduce memory allocation Mutation/PartitionUpdate logic:
* avoid an unnecessary byte buffer slicing (DataOutputBuffer.toByteArray())
during a Mutation serialization
* Replace AtomicLong viewLockAcquireStart field in Mutation with volatile
long and AtomicLongFieldUpdater usage|5%|Add new method to
DataOutputBuffer.unsafeToByteArray() (similar to a recently added
unsafeGetBufferAndFlip())|TBD|to merge to 5.0 in this ticket if possible or to
extract to a separate ticket to evaluate separately|
|4|Use simplified iterator logic for PartitionUpdate to avoid
LeafBTreeSearchIterator within inclusionTester() logic which checks if we need
to iterate over particular column for each cell. It is not really needed here:
we've just added these cells during PartitionUpdate creation|7.6%|Add new
builder method in ColumnFilter: all(RegularAndStaticColumns columns)|TBD|to
merge to 5.0 in this ticket if possible or to extract to a separate ticket to
evaluate separately|
|5|Cache BtreeRow.builder in a thread-local tiny pool to reuse in
UpdateParameters|4.6%|BTreeRow.pooledUnsortedBuilder method is added|TBD|to
merge to 5.0 in this ticket if possible or to extract to a separate ticket to
evaluate separately|
|6|Add optimized version of UpdatesCollector for a single partition update
within a single table (a very typical case when we just modify a row)|10%|No
existing API changes.
We introduce a new implementation for UpdatesCollector, theoretically we have
now 3 implementations of UpdatesCollector interface and
addUpdates() call can be megamorphic but it is not an issue, because:
1) multi-table batch is no a frequent case (so, in majority of cases the call
will be bimorphic)
2) addUpdates is big enough by itself and invoked once per partition, so the
overheads of a megamorphic call for this callsite (even if we have it) are
limited
3) benefits to not create and fill HashMultiset + 2 HashMap instances are much
higher |TBD|to merge to 5.0 in this ticket if possible or to extract to a
separate ticket to evaluate separately|
|7|Optimize a typical case: a single clustering key in an update|3%|Add new
method to MultiCBuilder.buildSingle()|TBD|Can be moved to 5.1 if needed|
|8|Avoid ImmutableList.copyOf(columnSpecs) within
OptionsWithColumnSpecifications constructor, we have to copy columnSpecs for
every request because in one place plain List is used and in another -
ImmutableList|2.3%|Change collection type from List to ImmutableList for a set
of methods in CQLStatement/QueryOptionsAdd a new method to
VariableSpecifications. A simple change but affects many classes.|TBD|Move to
5.1|
|9|iterate over restriction set as an array to avoid iterator usage|0.1%|Add
RestrictionSet.toArray() method|TBD|Move to 5.1 or drop due to a too small win|
note: I rounded % so the sum is slightly less than 100%
So, the suggestion:
# To merge to 5.0 in the context of the current story, options:
## minimal (1) row from the table; 60% of benefits, basic changes
## medium: (1) + (4) + (6); 77.6% of benefits
## bigger: (1) + (2) + (3) + (4) + (5) + (6) + (7)?; 95.7% of benefits
# (9) change I suggest to just drop due to a too small win by itself for the
amount of changes
# (8) or (7) + (8) - move to 5.1
# if something from (2) + (3) + (4) + (5) + (6) + (7) is too big in
combination with others for this story - we can evaluate them more precisely
part by part in separate stories (each change by itself is quite simple and
straightforward).
> Reduce memory allocations in miscellaneous places along write path
> ------------------------------------------------------------------
>
> Key: CASSANDRA-20167
> URL: https://issues.apache.org/jira/browse/CASSANDRA-20167
> Project: Apache Cassandra
> Issue Type: Improvement
> Components: Consistency/Coordination, Local/Other
> Reporter: Dmitry Konstantinov
> Assignee: Dmitry Konstantinov
> Priority: Normal
> Fix For: 5.0.x
>
> Attachments: 20167_BTreeRow_Builder_reuse-5.0.patch,
> 20167_SingleTableSinglePartitionUpdatesCollector-5.0.patch,
> 20167_bind_vars_immutable_list-5.0.patch, 20167_mutation-5.0.patch,
> 20167_network-5.0.patch, 20167_partition_update_iterator-5.0.patch,
> 20167_restriction_set_as_array-5.0.patch,
> 20167_single_clustering_key-5.0.patch,
> ci_summary_netudima_20167-5.0_147.html, image-2024-12-26-22-01-32-592.png,
> image-2024-12-26-22-01-55-614.png, image-2024-12-26-22-02-44-040.png,
> image-2024-12-26-22-03-49-018.png,
> results_details_netudima_20167-5.0_147.tar.xz
>
> Time Spent: 2h 10m
> Remaining Estimate: 0h
>
> Avoid unnecessary allocations in different places along write path:
> * avoid iterator allocations if possible
> * handle typical cases (such as a single row, single table writes) more
> efficiently
> * add fast paths for typical scenarios (like pending ranges is empty)
> * memorize things which can be computed once
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
