[
https://issues.apache.org/jira/browse/CASSANDRA-8897?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14506575#comment-14506575
]
Stefania commented on CASSANDRA-8897:
-------------------------------------
Here is the revised patch, please see last commit at
https://github.com/stef1927/cassandra/commits/8897, I did rebase to trunk.
I think I am going to keep the logging gards, in case we get round to inlining
the isXEnabled(), also I think there is still some
value as it seems int values are boxed:
{code}
L3
LINENUMBER 232 L3
INVOKESTATIC org/apache/cassandra/utils/memory/BufferPool.access$200
()Lorg/slf4j/Logger;
INVOKEINTERFACE org/slf4j/Logger.isTraceEnabled ()Z
IFEQ L4
L5
LINENUMBER 233 L5
INVOKESTATIC org/apache/cassandra/utils/memory/BufferPool.access$200
()Lorg/slf4j/Logger;
LDC "Requested heap buffer for {} bytes, allocating on heap"
ILOAD 1
INVOKESTATIC java/lang/Integer.valueOf (I)Ljava/lang/Integer;
INVOKEINTERFACE org/slf4j/Logger.trace
(Ljava/lang/String;Ljava/lang/Object;)V
{code}
bq. No, I mean in the scenario in which one thread does deallocate another
thread's buffer (by mistake we pass ownership) we could have a second count
variable that is updated only by threads that detect this scenario (and updated
via CAS), so that we can still maintain a safe count. However it is challenging
in this scenario to ensure we always detect when the count reaches full, so is
perhaps something to leave for another ticket (and only detect that we have
deallocated on another thread, log a panic, and accept we've leaked some
memory). If you'd like to address it in this ticket, do feel free though, since
it does give us some greater safety. We should log errors either way, since
this is not a target use case.
By count variable I assume you mean the free count. If so, I had a go at
implementing this but here are the difficulties I encountered:
- To protect against two threads releasing the buffer at the same time we would
need to attach an atomic boolean to the buffer, along with the chunk
and this atomic boolean would have to be cas-ed by the legitimate thread as
well, or lock on the buffer when we unattach the chunk.
- To to ensure we detect when the chunk is fully free, {{isFree()}} is no
longer very cheap since the ordinary count must become volatile, and the atomic
count must also be read (and its value is also a volatile int). So I think you
meant to keep {{isFree()}} extremely cheap and check in some other place?
Still, the ordinary count would have to be volatile surely?
You may want to give me more hints if I was completely wrong, or just revert
the count part and leave the warnings only (you find comments in the code, in
two places).
For the rest, we basically attach a thread id to the chunk to determine which
thread owns it, if any, and we attach the chunk to the buffers as you suggested.
I also implemented the middle deletion part, again if it is too wrong we can do
without, the only case I care is re-using a buffer returned immediately because
I think BigSSTableReader.getPosition() falls in this category when it loops
over segments. I decided to pack the two shorts into an int, initially because
I though it would be possible to sort without unpacking them, but as it turns
out we need to unpack the position before sorting so we could change it to an
array of 24 shorts rather than 12 ints, it doesn't really matter.
> Remove FileCacheService, instead pooling the buffers
> ----------------------------------------------------
>
> Key: CASSANDRA-8897
> URL: https://issues.apache.org/jira/browse/CASSANDRA-8897
> Project: Cassandra
> Issue Type: Improvement
> Components: Core
> Reporter: Benedict
> Assignee: Stefania
> Fix For: 3.0
>
>
> After CASSANDRA-8893, a RAR will be a very lightweight object and will not
> need caching, so we can eliminate this cache entirely. Instead we should have
> a pool of buffers that are page-aligned.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
