You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "netudima (via GitHub)" <gi...@apache.org> on 2024/12/26 22:12:59 UTC
[PR] [CASSANDRA-20167][5.0] Reduce memory allocations in miscellaneous places along write path [cassandra]
netudima opened a new pull request, #3761:
URL: https://github.com/apache/cassandra/pull/3761
Avoid unnecessary allocations in different places along write path, such as:
- 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
Patch by Dmitry Konstantinov; reviewed by TBD for CASSANDRA-20167
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] [CASSANDRA-20167][5.0] Reduce memory allocations in miscellaneous places along write path [cassandra]
Posted by "michaelsembwever (via GitHub)" <gi...@apache.org>.
michaelsembwever commented on code in PR #3761:
URL: https://github.com/apache/cassandra/pull/3761#discussion_r1910149356
##########
src/java/org/apache/cassandra/cql3/VariableSpecifications.java:
##########
@@ -56,6 +59,17 @@ public List<ColumnSpecification> getBindVariables()
return specs;
}
+ public ImmutableList<ColumnSpecification> getImmutableBindVariables()
+ {
+ ImmutableList<ColumnSpecification> result = immutableSpecs;
+ if (result == null) // strong syncrhronization is not needed, it is ok if sometimes we create several immutable lists
+ {
+ result = ImmutableList.copyOf(specs);
+ immutableSpecs = result;
+ }
+ return result;
Review Comment:
why not just
```suggestion
return immutableSpecs == null ? immutableSpecs = ImmutableList.copyOf(specs) : immutableSpecs;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] [CASSANDRA-20167][5.0] Reduce memory allocations in miscellaneous places along write path [cassandra]
Posted by "netudima (via GitHub)" <gi...@apache.org>.
netudima commented on code in PR #3761:
URL: https://github.com/apache/cassandra/pull/3761#discussion_r1901159994
##########
src/java/org/apache/cassandra/io/sstable/format/SortedTableWriter.java:
##########
@@ -270,22 +271,29 @@ protected void onStaticRow(Row row)
protected void onRow(Row row)
{
- notifyObservers(o -> o.nextUnfilteredCluster(row));
+ if (hasObservers())
+ notifyObservers(o -> o.nextUnfilteredCluster(row));
}
protected void onRangeTombstoneMarker(RangeTombstoneMarker marker)
{
- notifyObservers(o -> o.nextUnfilteredCluster(marker));
+ if (hasObservers())
+ notifyObservers(o -> o.nextUnfilteredCluster(marker));
}
protected abstract AbstractRowIndexEntry createRowIndexEntry(DecoratedKey key, DeletionTime partitionLevelDeletion, long finishResult) throws IOException;
protected final void notifyObservers(Consumer<SSTableFlushObserver> action)
{
- if (observers != null && !observers.isEmpty())
+ if (hasObservers())
observers.forEach(action);
}
+ private boolean hasObservers()
Review Comment:
Yes, as I see currently the only non-test implementations for observers are related to indexes, and indexes are a rare thing. In the methods we have capturing lamdas (with an externally captured variable value) which are allocated on each invocation, so I am trying to avoid them.
<img width="1294" alt="image" src="https://github.com/user-attachments/assets/9e87750b-7b1f-410f-8051-7f6178df440c" />
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] [CASSANDRA-20167][5.0] Reduce memory allocations in miscellaneous places along write path [cassandra]
Posted by "michaelsembwever (via GitHub)" <gi...@apache.org>.
michaelsembwever commented on code in PR #3761:
URL: https://github.com/apache/cassandra/pull/3761#discussion_r1910150593
##########
src/java/org/apache/cassandra/cql3/restrictions/PartitionKeySingleRestrictionSet.java:
##########
@@ -98,6 +106,11 @@ public List<ByteBuffer> values(QueryOptions options, ClientState state)
if (builder.hasMissingElements())
break;
}
+ // memory allocation optimization: to avoid BTreeSet, iterators allocation and use cheaper collections
+ // we handle a single clustering explicitly (we frequently modify a single row)
+ if (builder.buildSize() <= 1) {
+ return toByteBuffers(builder.buildSingle());
+ }
Review Comment:
```suggestion
if (builder.buildSize() <= 1)
return toByteBuffers(builder.buildSingle());
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] [CASSANDRA-20167][5.0] Reduce memory allocations in miscellaneous places along write path [cassandra]
Posted by "netudima (via GitHub)" <gi...@apache.org>.
netudima commented on code in PR #3761:
URL: https://github.com/apache/cassandra/pull/3761#discussion_r1901173587
##########
src/java/org/apache/cassandra/db/Mutation.java:
##########
@@ -448,7 +451,7 @@ private Serialization serialization(Mutation mutation, int version)
try (DataOutputBuffer dob = DataOutputBuffer.scratchBuffer.get())
{
serializeInternal(PartitionUpdate.serializer, mutation, dob, version);
- serialization = new CachedSerialization(dob.toByteArray());
+ serialization = new CachedSerialization(dob.unsafeToByteArray());
Review Comment:
If I find a cheap way to do it - I think I will create a separate improvement for it, it is more about CPU saving (while allocations are also affected a bit)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] [CASSANDRA-20167][5.0] Reduce memory allocations in miscellaneous places along write path [cassandra]
Posted by "clohfink (via GitHub)" <gi...@apache.org>.
clohfink commented on code in PR #3761:
URL: https://github.com/apache/cassandra/pull/3761#discussion_r1901152599
##########
src/java/org/apache/cassandra/io/sstable/format/SortedTableWriter.java:
##########
@@ -270,22 +271,29 @@ protected void onStaticRow(Row row)
protected void onRow(Row row)
{
- notifyObservers(o -> o.nextUnfilteredCluster(row));
+ if (hasObservers())
+ notifyObservers(o -> o.nextUnfilteredCluster(row));
}
protected void onRangeTombstoneMarker(RangeTombstoneMarker marker)
{
- notifyObservers(o -> o.nextUnfilteredCluster(marker));
+ if (hasObservers())
+ notifyObservers(o -> o.nextUnfilteredCluster(marker));
}
protected abstract AbstractRowIndexEntry createRowIndexEntry(DecoratedKey key, DeletionTime partitionLevelDeletion, long finishResult) throws IOException;
protected final void notifyObservers(Consumer<SSTableFlushObserver> action)
{
- if (observers != null && !observers.isEmpty())
+ if (hasObservers())
observers.forEach(action);
}
+ private boolean hasObservers()
Review Comment:
this is to avoid lambda creation correct? as only tables with indexes are expected to have observers?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] [CASSANDRA-20167][5.0] Reduce memory allocations in miscellaneous places along write path [cassandra]
Posted by "clohfink (via GitHub)" <gi...@apache.org>.
clohfink commented on code in PR #3761:
URL: https://github.com/apache/cassandra/pull/3761#discussion_r1901166470
##########
src/java/org/apache/cassandra/db/Mutation.java:
##########
@@ -448,7 +451,7 @@ private Serialization serialization(Mutation mutation, int version)
try (DataOutputBuffer dob = DataOutputBuffer.scratchBuffer.get())
{
serializeInternal(PartitionUpdate.serializer, mutation, dob, version);
- serialization = new CachedSerialization(dob.toByteArray());
+ serialization = new CachedSerialization(dob.unsafeToByteArray());
Review Comment:
if using on heap allocation can we also avoid byte array allocation here with buffer.array()? (buffer.hasArray()). Perhaps provide implementation of CachedSerialization that can use the backed array with offset/length. For larger mutations (ie 64kb-1mb) this can be pretty expensive still
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] [CASSANDRA-20167][5.0] Reduce memory allocations in miscellaneous places along write path [cassandra]
Posted by "clohfink (via GitHub)" <gi...@apache.org>.
clohfink commented on code in PR #3761:
URL: https://github.com/apache/cassandra/pull/3761#discussion_r1901168417
##########
src/java/org/apache/cassandra/db/Mutation.java:
##########
@@ -448,7 +451,7 @@ private Serialization serialization(Mutation mutation, int version)
try (DataOutputBuffer dob = DataOutputBuffer.scratchBuffer.get())
{
serializeInternal(PartitionUpdate.serializer, mutation, dob, version);
- serialization = new CachedSerialization(dob.toByteArray());
+ serialization = new CachedSerialization(dob.unsafeToByteArray());
Review Comment:
err never mind, since using the scratch buffer can't use backing array
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] [CASSANDRA-20167][5.0] Reduce memory allocations in miscellaneous places along write path [cassandra]
Posted by "clohfink (via GitHub)" <gi...@apache.org>.
clohfink commented on code in PR #3761:
URL: https://github.com/apache/cassandra/pull/3761#discussion_r1901128886
##########
src/java/org/apache/cassandra/cql3/CQLStatement.java:
##########
@@ -32,9 +34,9 @@ public interface CQLStatement
/**
* Returns all bind variables for the statement
*/
- default List<ColumnSpecification> getBindVariables()
+ default ImmutableList<ColumnSpecification> getBindVariables()
{
- return Collections.emptyList();
Review Comment:
Why switching this to immutable list? emptyList() would have same allocation profile right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] [CASSANDRA-20167][5.0] Reduce memory allocations in miscellaneous places along write path [cassandra]
Posted by "netudima (via GitHub)" <gi...@apache.org>.
netudima commented on code in PR #3761:
URL: https://github.com/apache/cassandra/pull/3761#discussion_r1901165468
##########
src/java/org/apache/cassandra/io/sstable/format/SortedTableWriter.java:
##########
@@ -270,22 +271,29 @@ protected void onStaticRow(Row row)
protected void onRow(Row row)
{
- notifyObservers(o -> o.nextUnfilteredCluster(row));
+ if (hasObservers())
+ notifyObservers(o -> o.nextUnfilteredCluster(row));
}
protected void onRangeTombstoneMarker(RangeTombstoneMarker marker)
{
- notifyObservers(o -> o.nextUnfilteredCluster(marker));
+ if (hasObservers())
+ notifyObservers(o -> o.nextUnfilteredCluster(marker));
}
protected abstract AbstractRowIndexEntry createRowIndexEntry(DecoratedKey key, DeletionTime partitionLevelDeletion, long finishResult) throws IOException;
protected final void notifyObservers(Consumer<SSTableFlushObserver> action)
{
- if (observers != null && !observers.isEmpty())
+ if (hasObservers())
observers.forEach(action);
}
+ private boolean hasObservers()
Review Comment:
of course, it is not a big saving by itself, but small amounts here and there can add up to something substantial overall.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] [CASSANDRA-20167][5.0] Reduce memory allocations in miscellaneous places along write path [cassandra]
Posted by "netudima (via GitHub)" <gi...@apache.org>.
netudima closed pull request #3761: [CASSANDRA-20167][5.0] Reduce memory allocations in miscellaneous places along write path
URL: https://github.com/apache/cassandra/pull/3761
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] [CASSANDRA-20167][5.0] Reduce memory allocations in miscellaneous places along write path [cassandra]
Posted by "netudima (via GitHub)" <gi...@apache.org>.
netudima commented on code in PR #3761:
URL: https://github.com/apache/cassandra/pull/3761#discussion_r1901133811
##########
src/java/org/apache/cassandra/cql3/CQLStatement.java:
##########
@@ -32,9 +34,9 @@ public interface CQLStatement
/**
* Returns all bind variables for the statement
*/
- default List<ColumnSpecification> getBindVariables()
+ default ImmutableList<ColumnSpecification> getBindVariables()
{
- return Collections.emptyList();
Review Comment:
The problem is with 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] [CASSANDRA-20167][5.0] Reduce memory allocations in miscellaneous places along write path [cassandra]
Posted by "netudima (via GitHub)" <gi...@apache.org>.
netudima commented on code in PR #3761:
URL: https://github.com/apache/cassandra/pull/3761#discussion_r1901172405
##########
src/java/org/apache/cassandra/db/Mutation.java:
##########
@@ -448,7 +451,7 @@ private Serialization serialization(Mutation mutation, int version)
try (DataOutputBuffer dob = DataOutputBuffer.scratchBuffer.get())
{
serializeInternal(PartitionUpdate.serializer, mutation, dob, version);
- serialization = new CachedSerialization(dob.toByteArray());
+ serialization = new CachedSerialization(dob.unsafeToByteArray());
Review Comment:
yes, it is reusable, so we have to copy the result
I have one non-implemented idea for this area of logic - initially we invoke serializedSize and then if the result is not too big we invoke the real serialization. In typical cases mutations are small and we can use some cheaper heuristic to decide if it is small to go directy to real serialization..
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] [CASSANDRA-20167][5.0] Reduce memory allocations in miscellaneous places along write path [cassandra]
Posted by "netudima (via GitHub)" <gi...@apache.org>.
netudima commented on code in PR #3761:
URL: https://github.com/apache/cassandra/pull/3761#discussion_r1910548979
##########
src/java/org/apache/cassandra/cql3/VariableSpecifications.java:
##########
@@ -56,6 +59,17 @@ public List<ColumnSpecification> getBindVariables()
return specs;
}
+ public ImmutableList<ColumnSpecification> getImmutableBindVariables()
+ {
+ ImmutableList<ColumnSpecification> result = immutableSpecs;
+ if (result == null) // strong syncrhronization is not needed, it is ok if sometimes we create several immutable lists
+ {
+ result = ImmutableList.copyOf(specs);
+ immutableSpecs = result;
+ }
+ return result;
Review Comment:
immutableSpecs is a volatile variable, in your case we read it 2 times, in my one - just once. It is a slightly better from performance point of view + a less error prone style when you compare and return the same reference which cannot be changed. It is a kind of typical pattern to deal with volatile variables (and sometimes even with plain fields). You can find it quite a lot in JDK code, for example:
```
Node<K, V>[] tab = this.table;
while(true) {
int n;
while(tab == null || (n = tab.length) == 0) {
tab = this.initTable();
}
```
from: java.util.concurrent.ConcurrentHashMap#putVal
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org