[
https://issues.apache.org/jira/browse/CASSANDRA-5863?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15256531#comment-15256531
]
Branimir Lambov commented on CASSANDRA-5863:
--------------------------------------------
Thanks for the review and ideas. Updated the branch to apply your comments.
bq. why does ReaderCache only account for buffer side instead of key + buffer
size in weighter, this means cache size is underestimated?
The key itself is a small and fixed part of the overhead (all objects it
references are already found elsewhere); there are also on-heap support
structures within the implementing cache which are bigger. Though that's not
trivial, we could also account for those, but I don't know how that helps cache
management and sizing for the user.
My point of view is that the user requests a certain amount of cached _data_
which is what the weigher currently measures. That enables a simple message to
the user (_x_ bytes of data off heap with _y_% typical on-heap overhead);
measuring (or accounting for) both would create difficulties communicating how
space is allocated and used.
bq. couple instanceof checks kind of signal that we want to re-evaluate
rebuffer class hierarchy.
You are right, there's no need this to be implemented outside the Rebufferer
hierarchy. Reorganized.
bq. ReaderCache#invalidateFile is not very efficient O\(n\) from the size of
the cache, which is used by cleanup of the mmap'ed files, which might be a
problem.
This needs to be evaluated in more detail to figure out if there's any point
calling it at all. It is necessary for testing, otherwise we could just as well
skip the call.
bq. (potential safety improvement) ChecksummedDataInput#readBuffer - should do
buffer vs. read length validation for -1 situation because otherwise this might
cause corruption
I'm sorry, I do not understand the problem -- the code only relies on the
position of the buffer and since buffer is cleared before the read, an end of
stream (and only that) will result in an empty buffer; both {{read()}} and
{{readByte()}} interpret this correctly.
bq. HintsReader - adds unused import and commented out seek which should be
removed
Fixed.
bq. since CRAR no longer extends RAR header comment about that should be
removed as well, as the matter of fact since CRAR now is just a container of
rebuffer implementations, maybe it makes sense to remove it all together and
just use RAR from CompressedSegmentedFile with different "rebuffer" backends,
so in other words put all of the rebuffers from CRAR to CSF?
Done, with a little extra clean-up around RAR.Builder.
bq. BufferlessRebufferer#rebuffer(long position, ByteBuffer buffer) at least
requires better clarification of the parameters and return value, because in
e.g. CRAR it's not exactly clear why would uncompressed buffer be provided to
also be returned, why can't argument just be filled and return type changed to
be long which is an (aligned) offset of the file?
I had added a return of the passed buffer for convenience but it also adds
possibility for error -- changed the return of the method to void. On the other
point, it does not make sense for the callee to return an (aligned) offset as
the caller may need to have a better control over positioning before allocating
the buffer -- caching rebufferers, specifically, do.
bq. Which allows to remove Rebufferer#rebuffer(long) method and always let
callers provide the buffer to fill, since I only see it used in RAR#reBufferAt
and LimitingRebufferer where both could be made to hold the actual buffer.
This wasn't the case even before this ticket. When RAR requests rebuffering at
a certain position, it can either have its buffer filled (direct case), or
receive a view of a shared buffer that holds the data (mem-mapped case). There
was a lot of clumsiness in RAR to handle the question of which of these is the
case, does it own its buffer, should it be allocated or freed. The patch
addresses this clumsiness as well as allowing for another type of advantageous
buffer management.
bq. Such allows to converge everything under BufferlessRebufferer and have
ReaderCache and RAR to handle buffers and divides reponsibilities of buffer
management and actual block storage handling between RAR and Rebufferer.
Unless you want the cache to copy data into a buffer provided by the RAR rather
than just provide access to a shared buffer, buffer management is integral in
caching and mem-mapped access and thus belongs in the rebufferer.
bq. make compressors always return a size of the buffer aligned on PAGE_SIZE
(default 512 bytes) and leave "holes" in the file
Interesting. Another possibility mentioned before is to implement compression
in such a way that the _compressed_ size matches the chunk size. Both are
orthogonal and outside the scope of this ticket -- lets open a new issue for
that?
> In process (uncompressed) page cache
> ------------------------------------
>
> Key: CASSANDRA-5863
> URL: https://issues.apache.org/jira/browse/CASSANDRA-5863
> Project: Cassandra
> Issue Type: Sub-task
> Reporter: T Jake Luciani
> Assignee: Branimir Lambov
> Labels: performance
> Fix For: 3.x
>
>
> Currently, for every read, the CRAR reads each compressed chunk into a
> byte[], sends it to ICompressor, gets back another byte[] and verifies a
> checksum.
> This process is where the majority of time is spent in a read request.
> Before compression, we would have zero-copy of data and could respond
> directly from the page-cache.
> It would be useful to have some kind of Chunk cache that could speed up this
> process for hot data, possibly off heap.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
