[
https://issues.apache.org/jira/browse/CASSANDRA-8793?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14347573#comment-14347573
]
Benedict commented on CASSANDRA-8793:
-------------------------------------
bq. SafeMemoryWriter.withByteOrder should be setByteOrder? I thought that
"with" implied that the original instance isn't changed and you get a new
version, but maybe I have the idiom wrong.
setX doesn't usually return self, but withX is a bit ambiguous; I've switched
to the semantics of BB, which just calls it "order"
bq. SafeMemoryWriter doesn't have a lot of \@Override annotations.
We tend not to use them (rightly or wrongly is a different question, I'm just
sticking to the standard practice)
bq. Implementing DataOutput, but outputting little endian breaks the contract
of the interface (or is it superclass) which is yikes. Guessing this isn't new
to this change.
DataOutput doesn't actually say _anything_ about endianness AFAICT. Java
default endianness is big endian, so I've defaulted SafeMemoryWriter to this
now, but obviously any direct unsafe modifications use native endianness
bq. withByte order was added even though it was already little endian or was
the previous code emitting the wrong byte order? Does that imply a test should
have caught it the change between versions of the code?
Previously there was no endianness correction in SafeMemoryWriter, and it was
writing directly to a Memory object via unsafe, i.e. it was writing in native
(little) endianness. This is correct for the situation in which we're using it,
but since the writer now defaults to Java's default bigendian format we must
explicitly request native format here.
bq. Bounds checking is done twice, once for ensure capacity, once for writing
to the buffer. If you could consolidate those say by calling a subclass method
on a failed bounds check that provides the policy then it would only have to
emit instructions for the check once?
I'm not keen on complicating the code here for a pretty small win. The
separation of concerns between the two classes would mean some ugliness to do
that, and if conditionally applying these checks we may not impose them when we
want to (after all, they should never be needed, right?) - and it's not a clear
benefit, at least with the current state of performance. With inlining it's
_possible_ the VM does this for us anyway.
bq. Memory.setShort invokes setBounds with the wrong second argument,
Fixed, thanks.
bq. Another argument for boring unit tests that check these boundary conditions.
Probably worth filing a separate ticket for jobs like this. We should start
curating a list of boring but necessary unit tests that we can work through,
but it's wider than the scope of this ticket and needs to be addressed
methodically in its own right, not just this one method.
> Avoid memory allocation when searching index summary
> ----------------------------------------------------
>
> Key: CASSANDRA-8793
> URL: https://issues.apache.org/jira/browse/CASSANDRA-8793
> Project: Cassandra
> Issue Type: Improvement
> Components: Core
> Reporter: Benedict
> Assignee: Benedict
> Priority: Minor
> Fix For: 3.0
>
>
> Currently we build a byte[] for each comparison, when we could just fill the
> details into a DirectByteBuffer
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
