[CASSANDRA-21040][trunk] Calculate once and cache the result of ModificationStatement#requiresRead as a perf optimization

[netudima]

Patch by Dmitry Konstantinov; reviewed by TBD for CASSANDRA-21040

[smiklosovic]

You can most probably do also this:

https://github.com/apache/cassandra/compare/trunk...smiklosovic:cassandra:CASSANDRA-21040-trunk?expand=1

If you just set that field earlier in the constructor, then you can just check if it requires any read for all operations and skip the additional iterations if not.

[smiklosovic]

I am not completely sure about

for (ReferenceOperation operation : operations.allSubstitutions())

refactoring, there is some additional logic for that method. But what requiresRead() method in Operations is doing is that it will iterate over this and iterator is going through both static and regular

@Override
public Iterator<Operation> iterator()
{
    return Iterators.concat(staticOperations.iterator(), regularOperations.iterator());
}

so it should be ok but needs to be double checked.

[smiklosovic]

what is not ideal about allSubstitutions() though is that it can potentially construct array list twice instead of just once if we hit that path, maybe this is more suitable in that case.

https://github.com/apache/cassandra/compare/trunk...smiklosovic:cassandra:CASSANDRA-21040-trunk-2nd-approach?expand=1

[netudima]

I am not sure actually if we need to try to optimize the constructor logic, it is executed once when a statement is preparing (and compared to the whole preparing logic it is a very small portion), so I tried to make the change as simple as possible just to get the perf benefits for hot path (prepared statements execution).

[smiklosovic]

yes it is true it is just in constructor and i was on an edge when proposing that ... i leave it up to your discretion