cassandra-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Branimir Lambov (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (CASSANDRA-5863) In process (uncompressed) page cache
Date Mon, 25 Apr 2016 16:10:13 GMT

    [ 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)

Mime
View raw message