You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by GitBox <gi...@apache.org> on 2020/09/08 04:39:24 UTC
[GitHub] [cassandra] bereng opened a new pull request #737: CASSANDRA-15880 ThreadLocal removal on close() to prevent leaks
bereng opened a new pull request #737:
URL: https://github.com/apache/cassandra/pull/737
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] bereng commented on a change in pull request #737: CASSANDRA-15880 ThreadLocal removal on close() to prevent leaks
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #737:
URL: https://github.com/apache/cassandra/pull/737#discussion_r490859645
##########
File path: src/java/org/apache/cassandra/db/commitlog/SimpleCachedBufferPool.java
##########
@@ -79,19 +91,20 @@ public ByteBuffer createBuffer(BufferType bufferType)
public ByteBuffer getThreadLocalReusableBuffer(int size)
{
- ByteBuffer result = reusableBufferHolder.get();
- if (result.capacity() < size || BufferType.typeOf(result) != preferredReusableBufferType)
+ ByteBuffer result = reusableBB.get();
+ if (result.capacity() < size)
{
FileUtils.clean(result);
result = preferredReusableBufferType.allocate(size);
- reusableBufferHolder.set(result);
+ reusableBB.set(result);
}
return result;
}
public void setPreferredReusableBufferType(BufferType type)
{
preferredReusableBufferType = type;
+ reusableBB = reusableBBHolder.get(preferredReusableBufferType);
Review comment:
Indeed the API for the pool could benefit from that imo as well. Adding the param to the constructor and making both the pool and the reusableBB follow that type sgtm and a better option overall. A quick look at the code seems to suggest BBType is reachable at construction time :-)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] bereng commented on a change in pull request #737: CASSANDRA-15880 ThreadLocal removal on close() to prevent leaks
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #737:
URL: https://github.com/apache/cassandra/pull/737#discussion_r494189562
##########
File path: src/java/org/apache/cassandra/db/commitlog/SimpleCachedBufferPool.java
##########
@@ -79,19 +91,20 @@ public ByteBuffer createBuffer(BufferType bufferType)
public ByteBuffer getThreadLocalReusableBuffer(int size)
{
- ByteBuffer result = reusableBufferHolder.get();
- if (result.capacity() < size || BufferType.typeOf(result) != preferredReusableBufferType)
+ ByteBuffer result = reusableBB.get();
+ if (result.capacity() < size)
{
FileUtils.clean(result);
result = preferredReusableBufferType.allocate(size);
- reusableBufferHolder.set(result);
+ reusableBB.set(result);
}
return result;
}
public void setPreferredReusableBufferType(BufferType type)
{
preferredReusableBufferType = type;
+ reusableBB = reusableBBHolder.get(preferredReusableBufferType);
Review comment:
@blerer suggested using a simple instance variable instead of a static one. The assumption is there will always only be one `AbstractCommitLogSegmentManager` instance. This allows for the imrpoved API and for tests to change config on the fly.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] blerer commented on a change in pull request #737: CASSANDRA-15880 ThreadLocal removal on close() to prevent leaks
Posted by GitBox <gi...@apache.org>.
blerer commented on a change in pull request #737:
URL: https://github.com/apache/cassandra/pull/737#discussion_r490823927
##########
File path: src/java/org/apache/cassandra/db/commitlog/SimpleCachedBufferPool.java
##########
@@ -79,19 +91,20 @@ public ByteBuffer createBuffer(BufferType bufferType)
public ByteBuffer getThreadLocalReusableBuffer(int size)
{
- ByteBuffer result = reusableBufferHolder.get();
- if (result.capacity() < size || BufferType.typeOf(result) != preferredReusableBufferType)
+ ByteBuffer result = reusableBB.get();
+ if (result.capacity() < size)
{
FileUtils.clean(result);
result = preferredReusableBufferType.allocate(size);
- reusableBufferHolder.set(result);
+ reusableBB.set(result);
}
return result;
}
public void setPreferredReusableBufferType(BufferType type)
{
preferredReusableBufferType = type;
+ reusableBB = reusableBBHolder.get(preferredReusableBufferType);
Review comment:
@bereng The approach of your last commit is also fine for me. I just tend to prefer the constructor approach because it avoid a lookup and makes the code easier to read (less parameters being passed around). That being said, I am open for both solutions.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] bereng commented on a change in pull request #737: CASSANDRA-15880 ThreadLocal removal on close() to prevent leaks
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #737:
URL: https://github.com/apache/cassandra/pull/737#discussion_r491810962
##########
File path: src/java/org/apache/cassandra/db/commitlog/SimpleCachedBufferPool.java
##########
@@ -79,19 +91,20 @@ public ByteBuffer createBuffer(BufferType bufferType)
public ByteBuffer getThreadLocalReusableBuffer(int size)
{
- ByteBuffer result = reusableBufferHolder.get();
- if (result.capacity() < size || BufferType.typeOf(result) != preferredReusableBufferType)
+ ByteBuffer result = reusableBB.get();
+ if (result.capacity() < size)
{
FileUtils.clean(result);
result = preferredReusableBufferType.allocate(size);
- reusableBufferHolder.set(result);
+ reusableBB.set(result);
}
return result;
}
public void setPreferredReusableBufferType(BufferType type)
{
preferredReusableBufferType = type;
+ reusableBB = reusableBBHolder.get(preferredReusableBufferType);
Review comment:
I have pushed the latest change where preferred BBType is set at construction time. Despite the API being much nicer imo, given we want to move away from singletons, static state/inits, etc I wonder if this is a step in the wrong direction.
Edit: Btw, the original implementation wasn't thread safe either, as `setPreferredReusableBufferType()` could be called by some other thread anytime changing things under your feet. That was a bug right?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] bereng commented on pull request #737: CASSANDRA-15880 ThreadLocal removal on close() to prevent leaks
Posted by GitBox <gi...@apache.org>.
bereng commented on pull request #737:
URL: https://github.com/apache/cassandra/pull/737#issuecomment-693309544
- [CI j11](https://app.circleci.com/pipelines/github/bereng/cassandra/114/workflows/339498da-4d60-435b-8c69-8d78e579a1e5)
- [Ci j8](https://app.circleci.com/pipelines/github/bereng/cassandra/114/workflows/04a5940e-5366-4609-8a21-f0521f024958)
CI lgtm so far pending running dtests
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] blerer commented on a change in pull request #737: CASSANDRA-15880 ThreadLocal removal on close() to prevent leaks
Posted by GitBox <gi...@apache.org>.
blerer commented on a change in pull request #737:
URL: https://github.com/apache/cassandra/pull/737#discussion_r490314377
##########
File path: src/java/org/apache/cassandra/db/commitlog/SimpleCachedBufferPool.java
##########
@@ -79,19 +91,20 @@ public ByteBuffer createBuffer(BufferType bufferType)
public ByteBuffer getThreadLocalReusableBuffer(int size)
{
- ByteBuffer result = reusableBufferHolder.get();
- if (result.capacity() < size || BufferType.typeOf(result) != preferredReusableBufferType)
+ ByteBuffer result = reusableBB.get();
+ if (result.capacity() < size)
{
FileUtils.clean(result);
result = preferredReusableBufferType.allocate(size);
- reusableBufferHolder.set(result);
+ reusableBB.set(result);
}
return result;
}
public void setPreferredReusableBufferType(BufferType type)
{
preferredReusableBufferType = type;
+ reusableBB = reusableBBHolder.get(preferredReusableBufferType);
Review comment:
You are right we can still have a race.
The preferedType is set in the `CompressedSegment` and int the `EncryptedSegment` constructor. I do not recall if we can change the commit log configuration without restarting the server. If not we can figure out which type will be the prefered one based on the `commitLog.configuration`. It is not super clean has it some how duplicate the logic from `createSegment` but it should work.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] bereng commented on a change in pull request #737: CASSANDRA-15880 ThreadLocal removal on close() to prevent leaks
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #737:
URL: https://github.com/apache/cassandra/pull/737#discussion_r489510546
##########
File path: src/java/org/apache/cassandra/db/commitlog/SimpleCachedBufferPool.java
##########
@@ -88,19 +91,21 @@ public ByteBuffer createBuffer(BufferType bufferType)
public ByteBuffer getThreadLocalReusableBuffer(int size)
{
- ByteBuffer result = reusableBBHolder.get(preferredReusableBufferType).get();
+ ByteBuffer result = reusableBB.get();
if (result.capacity() < size)
{
FileUtils.clean(result);
result = preferredReusableBufferType.allocate(size);
reusableBBHolder.get(preferredReusableBufferType).set(result);
Review comment:
right you are!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] bereng closed pull request #737: CASSANDRA-15880 ThreadLocal removal on close() to prevent leaks
Posted by GitBox <gi...@apache.org>.
bereng closed pull request #737:
URL: https://github.com/apache/cassandra/pull/737
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] bereng commented on a change in pull request #737: CASSANDRA-15880 ThreadLocal removal on close() to prevent leaks
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #737:
URL: https://github.com/apache/cassandra/pull/737#discussion_r491810962
##########
File path: src/java/org/apache/cassandra/db/commitlog/SimpleCachedBufferPool.java
##########
@@ -79,19 +91,20 @@ public ByteBuffer createBuffer(BufferType bufferType)
public ByteBuffer getThreadLocalReusableBuffer(int size)
{
- ByteBuffer result = reusableBufferHolder.get();
- if (result.capacity() < size || BufferType.typeOf(result) != preferredReusableBufferType)
+ ByteBuffer result = reusableBB.get();
+ if (result.capacity() < size)
{
FileUtils.clean(result);
result = preferredReusableBufferType.allocate(size);
- reusableBufferHolder.set(result);
+ reusableBB.set(result);
}
return result;
}
public void setPreferredReusableBufferType(BufferType type)
{
preferredReusableBufferType = type;
+ reusableBB = reusableBBHolder.get(preferredReusableBufferType);
Review comment:
I have pushed the latest change where preferred BBType is set at construction time. Despite the API being much nicer imo, given we want to move away from singletons, static state/inits, etc I wonder if this is a step in the wrong direction.
Edit: Btw, the original implementation wasn't thread safe either, as `setPreferredReusableBufferType()` could be called by some other thread anytime changing things under your feet. That was a bug right?
CI [fails](https://app.circleci.com/pipelines/github/bereng/cassandra/120/workflows/7f806bb7-4339-421f-9486-ecc47d073afd/jobs/916): Investigating
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] bereng commented on pull request #737: CASSANDRA-15880 ThreadLocal removal on close() to prevent leaks
Posted by GitBox <gi...@apache.org>.
bereng commented on pull request #737:
URL: https://github.com/apache/cassandra/pull/737#issuecomment-693231718
CI
- [j11](https://app.circleci.com/pipelines/github/bereng/cassandra/112/workflows/dadee965-c951-49ec-a10c-02b7faaaffff)
- [j8](https://app.circleci.com/pipelines/github/bereng/cassandra/112/workflows/c1a6addd-977e-4d42-a1e0-05928e25f46f)
CI LGTM
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] bereng commented on a change in pull request #737: CASSANDRA-15880 ThreadLocal removal on close() to prevent leaks
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #737:
URL: https://github.com/apache/cassandra/pull/737#discussion_r494189562
##########
File path: src/java/org/apache/cassandra/db/commitlog/SimpleCachedBufferPool.java
##########
@@ -79,19 +91,20 @@ public ByteBuffer createBuffer(BufferType bufferType)
public ByteBuffer getThreadLocalReusableBuffer(int size)
{
- ByteBuffer result = reusableBufferHolder.get();
- if (result.capacity() < size || BufferType.typeOf(result) != preferredReusableBufferType)
+ ByteBuffer result = reusableBB.get();
+ if (result.capacity() < size)
{
FileUtils.clean(result);
result = preferredReusableBufferType.allocate(size);
- reusableBufferHolder.set(result);
+ reusableBB.set(result);
}
return result;
}
public void setPreferredReusableBufferType(BufferType type)
{
preferredReusableBufferType = type;
+ reusableBB = reusableBBHolder.get(preferredReusableBufferType);
Review comment:
@blerer suggested using a simple instance variable instead of a static one. The assumption is there will always only be one `AbstractCommitLogSegmentManager` instance. This allows for the imrpoved API and for tests to change config on the fly.
##########
File path: src/java/org/apache/cassandra/db/commitlog/SimpleCachedBufferPool.java
##########
@@ -79,19 +91,20 @@ public ByteBuffer createBuffer(BufferType bufferType)
public ByteBuffer getThreadLocalReusableBuffer(int size)
{
- ByteBuffer result = reusableBufferHolder.get();
- if (result.capacity() < size || BufferType.typeOf(result) != preferredReusableBufferType)
+ ByteBuffer result = reusableBB.get();
+ if (result.capacity() < size)
{
FileUtils.clean(result);
result = preferredReusableBufferType.allocate(size);
- reusableBufferHolder.set(result);
+ reusableBB.set(result);
}
return result;
}
public void setPreferredReusableBufferType(BufferType type)
{
preferredReusableBufferType = type;
+ reusableBB = reusableBBHolder.get(preferredReusableBufferType);
Review comment:
@blerer suggested using a simple instance variable instead of a static one. The assumption is there will always only be one `AbstractCommitLogSegmentManager` instance. This allows for the imrpoved API and for tests to change config on the fly. Latest commit available.
##########
File path: src/java/org/apache/cassandra/db/commitlog/AbstractCommitLogSegmentManager.java
##########
@@ -166,7 +148,22 @@ public void runMayThrow() throws Exception
}
};
+ BufferType BBType = SimpleCachedBufferPool.DEFAULT_PREFERRED_BB_TYPE;
+ if (commitLog.configuration.useEncryption())
+ // Keep reusable buffers on-heap regardless of compression preference so we avoid copy off/on repeatedly during decryption
+ // Also: we want to keep the compression buffers on-heap as we need those bytes for encryption,
+ // and we want to avoid copying from off-heap (compression buffer) to on-heap encryption APIs
+ BBType = BufferType.ON_HEAP;
+ else if (commitLog.configuration.useCompression())
+ BBType = commitLog.configuration.getCompressor().preferredBufferType();
+
+ synchronized(this)
+ {
+ this.bufferPool = new SimpleCachedBufferPool(DatabaseDescriptor.getCommitLogMaxCompressionBuffersInPool(), DatabaseDescriptor.getCommitLogSegmentSize(), BBType);
Review comment:
We can make this more 'solid' by closing the pool before re-creation to defend against `start()` calls without a matching ' close()`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] bereng commented on a change in pull request #737: CASSANDRA-15880 ThreadLocal removal on close() to prevent leaks
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #737:
URL: https://github.com/apache/cassandra/pull/737#discussion_r491810962
##########
File path: src/java/org/apache/cassandra/db/commitlog/SimpleCachedBufferPool.java
##########
@@ -79,19 +91,20 @@ public ByteBuffer createBuffer(BufferType bufferType)
public ByteBuffer getThreadLocalReusableBuffer(int size)
{
- ByteBuffer result = reusableBufferHolder.get();
- if (result.capacity() < size || BufferType.typeOf(result) != preferredReusableBufferType)
+ ByteBuffer result = reusableBB.get();
+ if (result.capacity() < size)
{
FileUtils.clean(result);
result = preferredReusableBufferType.allocate(size);
- reusableBufferHolder.set(result);
+ reusableBB.set(result);
}
return result;
}
public void setPreferredReusableBufferType(BufferType type)
{
preferredReusableBufferType = type;
+ reusableBB = reusableBBHolder.get(preferredReusableBufferType);
Review comment:
I have pushed the latest change where preferred BBType is set at construction time. Despite the API being much nicer imo, given we want to move away from singletons, static state/inits, etc I wonder if this is a step in the wrong direction.
##########
File path: src/java/org/apache/cassandra/db/commitlog/SimpleCachedBufferPool.java
##########
@@ -79,19 +91,20 @@ public ByteBuffer createBuffer(BufferType bufferType)
public ByteBuffer getThreadLocalReusableBuffer(int size)
{
- ByteBuffer result = reusableBufferHolder.get();
- if (result.capacity() < size || BufferType.typeOf(result) != preferredReusableBufferType)
+ ByteBuffer result = reusableBB.get();
+ if (result.capacity() < size)
{
FileUtils.clean(result);
result = preferredReusableBufferType.allocate(size);
- reusableBufferHolder.set(result);
+ reusableBB.set(result);
}
return result;
}
public void setPreferredReusableBufferType(BufferType type)
{
preferredReusableBufferType = type;
+ reusableBB = reusableBBHolder.get(preferredReusableBufferType);
Review comment:
I have pushed the latest change where preferred BBType is set at construction time. Despite the API being much nicer imo, given we want to move away from singletons, static state/inits, etc I wonder if this is a step in the wrong direction.
Edit: Btw, the original implementation wasn't thread safe either, as `setPreferredReusableBufferType()` could be called by some other thread anytime changing things under your feet. That was a bug right?
##########
File path: src/java/org/apache/cassandra/db/commitlog/SimpleCachedBufferPool.java
##########
@@ -79,19 +91,20 @@ public ByteBuffer createBuffer(BufferType bufferType)
public ByteBuffer getThreadLocalReusableBuffer(int size)
{
- ByteBuffer result = reusableBufferHolder.get();
- if (result.capacity() < size || BufferType.typeOf(result) != preferredReusableBufferType)
+ ByteBuffer result = reusableBB.get();
+ if (result.capacity() < size)
{
FileUtils.clean(result);
result = preferredReusableBufferType.allocate(size);
- reusableBufferHolder.set(result);
+ reusableBB.set(result);
}
return result;
}
public void setPreferredReusableBufferType(BufferType type)
{
preferredReusableBufferType = type;
+ reusableBB = reusableBBHolder.get(preferredReusableBufferType);
Review comment:
I have pushed the latest change where preferred BBType is set at construction time. Despite the API being much nicer imo, given we want to move away from singletons, static state/inits, etc I wonder if this is a step in the wrong direction.
Edit: Btw, the original implementation wasn't thread safe either, as `setPreferredReusableBufferType()` could be called by some other thread anytime changing things under your feet. That was a bug right?
CI [fails](https://app.circleci.com/pipelines/github/bereng/cassandra/120/workflows/7f806bb7-4339-421f-9486-ecc47d073afd/jobs/916): Investigating
##########
File path: src/java/org/apache/cassandra/db/commitlog/SimpleCachedBufferPool.java
##########
@@ -79,19 +91,20 @@ public ByteBuffer createBuffer(BufferType bufferType)
public ByteBuffer getThreadLocalReusableBuffer(int size)
{
- ByteBuffer result = reusableBufferHolder.get();
- if (result.capacity() < size || BufferType.typeOf(result) != preferredReusableBufferType)
+ ByteBuffer result = reusableBB.get();
+ if (result.capacity() < size)
{
FileUtils.clean(result);
result = preferredReusableBufferType.allocate(size);
- reusableBufferHolder.set(result);
+ reusableBB.set(result);
}
return result;
}
public void setPreferredReusableBufferType(BufferType type)
{
preferredReusableBufferType = type;
+ reusableBB = reusableBBHolder.get(preferredReusableBufferType);
Review comment:
So apparently yes, it was a step in the 'wrong' direction. We have tests that apparently change config [on the fly](https://github.com/apache/cassandra/blob/trunk/test/unit/org/apache/cassandra/db/RecoveryManagerFlushedTest.java#L71). So the static init [won't play nice](https://app.circleci.com/pipelines/github/bereng/cassandra/120/workflows/7f806bb7-4339-421f-9486-ecc47d073afd/jobs/916) with that.
Hence I propose we settle on the solution with the `EnumMap` [lookup](https://github.com/apache/cassandra/commit/1e4b2f483d221bb1adce212dac4b2109ca0e9789)
wdyt?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] bereng commented on a change in pull request #737: CASSANDRA-15880 ThreadLocal removal on close() to prevent leaks
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #737:
URL: https://github.com/apache/cassandra/pull/737#discussion_r489439285
##########
File path: src/java/org/apache/cassandra/db/commitlog/SimpleCachedBufferPool.java
##########
@@ -79,12 +88,12 @@ public ByteBuffer createBuffer(BufferType bufferType)
public ByteBuffer getThreadLocalReusableBuffer(int size)
{
- ByteBuffer result = reusableBufferHolder.get();
- if (result.capacity() < size || BufferType.typeOf(result) != preferredReusableBufferType)
+ ByteBuffer result = reusableBBHolder.get(preferredReusableBufferType).get();
Review comment:
Not so nit imo. Getting the buffer is sure more prone to being part of a hot path than changing the type. So fiddly but I gave it a go.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] bereng commented on pull request #737: CASSANDRA-15880 ThreadLocal removal on close() to prevent leaks
Posted by GitBox <gi...@apache.org>.
bereng commented on pull request #737:
URL: https://github.com/apache/cassandra/pull/737#issuecomment-693237694
@blambov suggested the `EnumMap` addition to prevent BB recreation on different BBTypes. If you had a gap to cursory look this change that'd be great. I still preferred using `SimpleCachedBufferPool` as a centralized point to get ThreadLocal BB from.
@blerer does this now look better?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] bereng commented on pull request #737: CASSANDRA-15880 ThreadLocal removal on close() to prevent leaks
Posted by GitBox <gi...@apache.org>.
bereng commented on pull request #737:
URL: https://github.com/apache/cassandra/pull/737#issuecomment-688692635
CI
- [j8](https://app.circleci.com/pipelines/github/bereng/cassandra/101/workflows/5056ddf5-693d-4b2e-b08d-983253eeed6e): All failures are the the latest usual flaky tests
- [j11](https://app.circleci.com/pipelines/github/bereng/cassandra/101/workflows/7d02ef6e-c7f2-4fa1-b2ec-dcbeba6da0bc): The only non-usual-suspect failure, test_13595 - consistency_test.TestConsistency, passes locally.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] blambov commented on a change in pull request #737: CASSANDRA-15880 ThreadLocal removal on close() to prevent leaks
Posted by GitBox <gi...@apache.org>.
blambov commented on a change in pull request #737:
URL: https://github.com/apache/cassandra/pull/737#discussion_r490259247
##########
File path: src/java/org/apache/cassandra/db/commitlog/SimpleCachedBufferPool.java
##########
@@ -79,19 +91,20 @@ public ByteBuffer createBuffer(BufferType bufferType)
public ByteBuffer getThreadLocalReusableBuffer(int size)
{
- ByteBuffer result = reusableBufferHolder.get();
- if (result.capacity() < size || BufferType.typeOf(result) != preferredReusableBufferType)
+ ByteBuffer result = reusableBB.get();
+ if (result.capacity() < size)
{
FileUtils.clean(result);
result = preferredReusableBufferType.allocate(size);
- reusableBufferHolder.set(result);
+ reusableBB.set(result);
}
return result;
}
public void setPreferredReusableBufferType(BufferType type)
{
preferredReusableBufferType = type;
+ reusableBB = reusableBBHolder.get(preferredReusableBufferType);
Review comment:
Making them volatile won't change much, the right thing to do is to not allow the preferred type to change. Is it possible to pass the type as a constructor argument and make `reuseableBB` final?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] blerer commented on a change in pull request #737: CASSANDRA-15880 ThreadLocal removal on close() to prevent leaks
Posted by GitBox <gi...@apache.org>.
blerer commented on a change in pull request #737:
URL: https://github.com/apache/cassandra/pull/737#discussion_r490816897
##########
File path: src/java/org/apache/cassandra/db/commitlog/SimpleCachedBufferPool.java
##########
@@ -79,19 +91,20 @@ public ByteBuffer createBuffer(BufferType bufferType)
public ByteBuffer getThreadLocalReusableBuffer(int size)
{
- ByteBuffer result = reusableBufferHolder.get();
- if (result.capacity() < size || BufferType.typeOf(result) != preferredReusableBufferType)
+ ByteBuffer result = reusableBB.get();
+ if (result.capacity() < size)
{
FileUtils.clean(result);
result = preferredReusableBufferType.allocate(size);
- reusableBufferHolder.set(result);
+ reusableBB.set(result);
}
return result;
}
public void setPreferredReusableBufferType(BufferType type)
{
preferredReusableBufferType = type;
+ reusableBB = reusableBBHolder.get(preferredReusableBufferType);
Review comment:
You are right, there is an issue with the class API. Even with the `setPreferredReusableBufferType` the API is confusing as it only applies to the `ThreadLocal` logic and not to the buffer pool. The calls to `createBuffer` are also performed from within the `CompressedSegment` and `EncryptedSegment` using the same type as the `preferredReusableBufferType`. So we can pass the buffer type as part of the constructor and remove the parameter from `createBuffer`. It will fix the threading issue and the API at the same time. Wdyt?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] bereng commented on pull request #737: CASSANDRA-15880 ThreadLocal removal on close() to prevent leaks
Posted by GitBox <gi...@apache.org>.
bereng commented on pull request #737:
URL: https://github.com/apache/cassandra/pull/737#issuecomment-702532358
A few failures I need to investigate with the latest CI run [J11](https://app.circleci.com/pipelines/github/bereng/cassandra/136/workflows/6e1808b7-224a-4702-bf19-944250365599) [j8](https://app.circleci.com/pipelines/github/bereng/cassandra/136/workflows/92fffdea-31a0-4679-a463-f6261ccf4a4b)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] blambov commented on a change in pull request #737: CASSANDRA-15880 ThreadLocal removal on close() to prevent leaks
Posted by GitBox <gi...@apache.org>.
blambov commented on a change in pull request #737:
URL: https://github.com/apache/cassandra/pull/737#discussion_r489237475
##########
File path: src/java/org/apache/cassandra/io/util/CompressedChunkReader.java
##########
@@ -85,28 +88,28 @@ public Rebufferer instantiateRebufferer()
public static class Standard extends CompressedChunkReader
{
- // we read the raw compressed bytes into this buffer, then uncompressed them into the provided one.
- private final ThreadLocal<ByteBuffer> compressedHolder;
+ // We read the raw compressed bytes into a buffer, then uncompressed them into the provided one.
+ // Notice we have 1 SimpleCachedBufferPool per BBType which wraps a FastThreadLocal BB
+ private static final EnumMap<BufferType, SimpleCachedBufferPool> reusableCompressBBs = new EnumMap<>(BufferType.class);
Review comment:
This does not fix having to recreate buffers if the type changes, because the underlying thread local map is still a single static.
I would use the static `EnumMap` in `SimpleCachedBufferPool` instead.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] bereng commented on a change in pull request #737: CASSANDRA-15880 ThreadLocal removal on close() to prevent leaks
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #737:
URL: https://github.com/apache/cassandra/pull/737#discussion_r494189562
##########
File path: src/java/org/apache/cassandra/db/commitlog/SimpleCachedBufferPool.java
##########
@@ -79,19 +91,20 @@ public ByteBuffer createBuffer(BufferType bufferType)
public ByteBuffer getThreadLocalReusableBuffer(int size)
{
- ByteBuffer result = reusableBufferHolder.get();
- if (result.capacity() < size || BufferType.typeOf(result) != preferredReusableBufferType)
+ ByteBuffer result = reusableBB.get();
+ if (result.capacity() < size)
{
FileUtils.clean(result);
result = preferredReusableBufferType.allocate(size);
- reusableBufferHolder.set(result);
+ reusableBB.set(result);
}
return result;
}
public void setPreferredReusableBufferType(BufferType type)
{
preferredReusableBufferType = type;
+ reusableBB = reusableBBHolder.get(preferredReusableBufferType);
Review comment:
@blerer suggested using a simple instance variable instead of a static one. The assumption is there will always only be one `AbstractCommitLogSegmentManager` instance. This allows for the imrpoved API and for tests to change config on the fly. Latest commit available.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] blambov commented on a change in pull request #737: CASSANDRA-15880 ThreadLocal removal on close() to prevent leaks
Posted by GitBox <gi...@apache.org>.
blambov commented on a change in pull request #737:
URL: https://github.com/apache/cassandra/pull/737#discussion_r490024486
##########
File path: src/java/org/apache/cassandra/db/commitlog/SimpleCachedBufferPool.java
##########
@@ -63,6 +74,7 @@ public SimpleCachedBufferPool(int maxBufferPoolSize, int bufferSize)
{
this.maxBufferPoolSize = maxBufferPoolSize;
this.bufferSize = bufferSize;
+ SimpleCachedBufferPool.reusableBB = reusableBBHolder.get(preferredReusableBufferType);
Review comment:
Nit: replace `SimpleCachedBufferPool` with `this`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] blerer commented on a change in pull request #737: CASSANDRA-15880 ThreadLocal removal on close() to prevent leaks
Posted by GitBox <gi...@apache.org>.
blerer commented on a change in pull request #737:
URL: https://github.com/apache/cassandra/pull/737#discussion_r490232796
##########
File path: src/java/org/apache/cassandra/db/commitlog/SimpleCachedBufferPool.java
##########
@@ -79,19 +91,20 @@ public ByteBuffer createBuffer(BufferType bufferType)
public ByteBuffer getThreadLocalReusableBuffer(int size)
{
- ByteBuffer result = reusableBufferHolder.get();
- if (result.capacity() < size || BufferType.typeOf(result) != preferredReusableBufferType)
+ ByteBuffer result = reusableBB.get();
+ if (result.capacity() < size)
{
FileUtils.clean(result);
result = preferredReusableBufferType.allocate(size);
- reusableBufferHolder.set(result);
+ reusableBB.set(result);
}
return result;
}
public void setPreferredReusableBufferType(BufferType type)
{
preferredReusableBufferType = type;
+ reusableBB = reusableBBHolder.get(preferredReusableBufferType);
Review comment:
Should we not have `preferredReusableBufferType` and `reusableBB` be `volatile` variables and the `setPreferredReusableBufferType` method `synchronized` ? With the current implementation, it seems to me that, if a thread call `setPreferredReusableBufferType` and another `getThreadLocalReusableBuffer` the output of `getThreadLocalReusableBuffer` is unpredictable and the wrong buffer type can be added the `reusableBB` field.
Am I missing something?
##########
File path: src/java/org/apache/cassandra/db/commitlog/SimpleCachedBufferPool.java
##########
@@ -107,6 +120,9 @@ public void releaseBuffer(ByteBuffer buffer)
public void shutdown()
{
bufferPool.clear();
+ for (FastThreadLocal<ByteBuffer> bbHolder : reusableBBHolder.values())
+ bbHolder.remove();
+ reusableBB.remove();
Review comment:
`remove` is called twice on the same `ThreadLocal` instance as the `reusableBB` is also in the `bbHolder` variable
Moreover, the `remove` call will remove the buffers values associated to the thread calling the `shutdown` method and I do not believe that it is what we want. The goal of the approach is to reuse buffer. Calling `remove` will prevent the reuse on the next call forcing the buffer to be recreated.
##########
File path: src/java/org/apache/cassandra/io/util/CompressedChunkReader.java
##########
@@ -86,27 +87,21 @@ public Rebufferer instantiateRebufferer()
public static class Standard extends CompressedChunkReader
{
// we read the raw compressed bytes into this buffer, then uncompressed them into the provided one.
- private final ThreadLocal<ByteBuffer> compressedHolder;
+ private final SimpleCachedBufferPool reusableCompressBB;
+ private final int compressSize = getCompressSize();
public Standard(ChannelProxy channel, CompressionMetadata metadata)
{
super(channel, metadata);
- compressedHolder = ThreadLocal.withInitial(this::allocateBuffer);
+ reusableCompressBB = new SimpleCachedBufferPool(0, getCompressSize());
Review comment:
We compute twice the compress size. I should do something like:
```
compressSize = getCompressSize(metadata);
reusableCompressBB = new SimpleCachedBufferPool(0, compressSize);
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] bereng commented on a change in pull request #737: CASSANDRA-15880 ThreadLocal removal on close() to prevent leaks
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #737:
URL: https://github.com/apache/cassandra/pull/737#discussion_r490771906
##########
File path: src/java/org/apache/cassandra/db/commitlog/SimpleCachedBufferPool.java
##########
@@ -79,19 +91,20 @@ public ByteBuffer createBuffer(BufferType bufferType)
public ByteBuffer getThreadLocalReusableBuffer(int size)
{
- ByteBuffer result = reusableBufferHolder.get();
- if (result.capacity() < size || BufferType.typeOf(result) != preferredReusableBufferType)
+ ByteBuffer result = reusableBB.get();
+ if (result.capacity() < size)
{
FileUtils.clean(result);
result = preferredReusableBufferType.allocate(size);
- reusableBufferHolder.set(result);
+ reusableBB.set(result);
}
return result;
}
public void setPreferredReusableBufferType(BufferType type)
{
preferredReusableBufferType = type;
+ reusableBB = reusableBBHolder.get(preferredReusableBufferType);
Review comment:
Given this `getThreadLocalReusableBuffer` seems to be isolated within the class, as it is not tied in with the buffer pool functinality, I don't like adding this param to the constructor. It reads as if this was the preferred type to the BBs in the pool... sounds confusing to me, changes the 'main' API, it might not get used if you only use the pool functionality
I would like to propose the latest commit which does away with both variables. The price to pay is the Map lookup. I read a bit about `EnumMap`'s implementation and it is an Array indexed by the Enum int. So it should be pretty fast imo.
Wdyt? you guys know better how this class plays within the hot-paths.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] blambov commented on a change in pull request #737: CASSANDRA-15880 ThreadLocal removal on close() to prevent leaks
Posted by GitBox <gi...@apache.org>.
blambov commented on a change in pull request #737:
URL: https://github.com/apache/cassandra/pull/737#discussion_r489409378
##########
File path: src/java/org/apache/cassandra/db/commitlog/SimpleCachedBufferPool.java
##########
@@ -79,12 +88,12 @@ public ByteBuffer createBuffer(BufferType bufferType)
public ByteBuffer getThreadLocalReusableBuffer(int size)
{
- ByteBuffer result = reusableBufferHolder.get();
- if (result.capacity() < size || BufferType.typeOf(result) != preferredReusableBufferType)
+ ByteBuffer result = reusableBBHolder.get(preferredReusableBufferType).get();
Review comment:
Nit: Instead of `preferredReusableBufferType` we can store the result of `reusableBBHolder.get(preferredReusableBufferType)` on `setPreferredReusableBufferType` calls to avoid the `EnumMap` lookup.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] blambov commented on a change in pull request #737: CASSANDRA-15880 ThreadLocal removal on close() to prevent leaks
Posted by GitBox <gi...@apache.org>.
blambov commented on a change in pull request #737:
URL: https://github.com/apache/cassandra/pull/737#discussion_r489478448
##########
File path: src/java/org/apache/cassandra/db/commitlog/SimpleCachedBufferPool.java
##########
@@ -37,6 +37,8 @@
public class SimpleCachedBufferPool
{
private static final EnumMap<BufferType, FastThreadLocal<ByteBuffer>> reusableBBHolder = new EnumMap<>(BufferType.class);
+ // Convenience variable holding a ref to the current resuableBB to avoid map lookups
+ private static FastThreadLocal<ByteBuffer> reusableBB;
Review comment:
This should not be static.
##########
File path: src/java/org/apache/cassandra/db/commitlog/SimpleCachedBufferPool.java
##########
@@ -88,19 +91,21 @@ public ByteBuffer createBuffer(BufferType bufferType)
public ByteBuffer getThreadLocalReusableBuffer(int size)
{
- ByteBuffer result = reusableBBHolder.get(preferredReusableBufferType).get();
+ ByteBuffer result = reusableBB.get();
if (result.capacity() < size)
{
FileUtils.clean(result);
result = preferredReusableBufferType.allocate(size);
reusableBBHolder.get(preferredReusableBufferType).set(result);
Review comment:
This is now redundant.
##########
File path: src/java/org/apache/cassandra/db/commitlog/SimpleCachedBufferPool.java
##########
@@ -72,6 +74,7 @@ public SimpleCachedBufferPool(int maxBufferPoolSize, int bufferSize)
{
Review comment:
`preferredReusableBufferType` above should no longer be necessary.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] bereng commented on a change in pull request #737: CASSANDRA-15880 ThreadLocal removal on close() to prevent leaks
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #737:
URL: https://github.com/apache/cassandra/pull/737#discussion_r491913063
##########
File path: src/java/org/apache/cassandra/db/commitlog/SimpleCachedBufferPool.java
##########
@@ -79,19 +91,20 @@ public ByteBuffer createBuffer(BufferType bufferType)
public ByteBuffer getThreadLocalReusableBuffer(int size)
{
- ByteBuffer result = reusableBufferHolder.get();
- if (result.capacity() < size || BufferType.typeOf(result) != preferredReusableBufferType)
+ ByteBuffer result = reusableBB.get();
+ if (result.capacity() < size)
{
FileUtils.clean(result);
result = preferredReusableBufferType.allocate(size);
- reusableBufferHolder.set(result);
+ reusableBB.set(result);
}
return result;
}
public void setPreferredReusableBufferType(BufferType type)
{
preferredReusableBufferType = type;
+ reusableBB = reusableBBHolder.get(preferredReusableBufferType);
Review comment:
So apparently yes, it was a step in the 'wrong' direction. We have tests that apparently change config [on the fly](https://github.com/apache/cassandra/blob/trunk/test/unit/org/apache/cassandra/db/RecoveryManagerFlushedTest.java#L71). So the static init [won't play nice](https://app.circleci.com/pipelines/github/bereng/cassandra/120/workflows/7f806bb7-4339-421f-9486-ecc47d073afd/jobs/916) with that.
Hence I propose we settle on the solution with the `EnumMap` [lookup](https://github.com/apache/cassandra/commit/1e4b2f483d221bb1adce212dac4b2109ca0e9789)
wdyt?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] bereng commented on a change in pull request #737: CASSANDRA-15880 ThreadLocal removal on close() to prevent leaks
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #737:
URL: https://github.com/apache/cassandra/pull/737#discussion_r491810962
##########
File path: src/java/org/apache/cassandra/db/commitlog/SimpleCachedBufferPool.java
##########
@@ -79,19 +91,20 @@ public ByteBuffer createBuffer(BufferType bufferType)
public ByteBuffer getThreadLocalReusableBuffer(int size)
{
- ByteBuffer result = reusableBufferHolder.get();
- if (result.capacity() < size || BufferType.typeOf(result) != preferredReusableBufferType)
+ ByteBuffer result = reusableBB.get();
+ if (result.capacity() < size)
{
FileUtils.clean(result);
result = preferredReusableBufferType.allocate(size);
- reusableBufferHolder.set(result);
+ reusableBB.set(result);
}
return result;
}
public void setPreferredReusableBufferType(BufferType type)
{
preferredReusableBufferType = type;
+ reusableBB = reusableBBHolder.get(preferredReusableBufferType);
Review comment:
I have pushed the latest change where preferred BBType is set at construction time. Despite the API being much nicer imo, given we want to move away from singletons, static state/inits, etc I wonder if this is a step in the wrong direction.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] blambov commented on pull request #737: CASSANDRA-15880 ThreadLocal removal on close() to prevent leaks
Posted by GitBox <gi...@apache.org>.
blambov commented on pull request #737:
URL: https://github.com/apache/cassandra/pull/737#issuecomment-702704290
Looks great now. Thanks for the patience.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] bereng commented on a change in pull request #737: CASSANDRA-15880 ThreadLocal removal on close() to prevent leaks
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #737:
URL: https://github.com/apache/cassandra/pull/737#discussion_r489967748
##########
File path: src/java/org/apache/cassandra/db/commitlog/SimpleCachedBufferPool.java
##########
@@ -37,6 +37,8 @@
public class SimpleCachedBufferPool
{
private static final EnumMap<BufferType, FastThreadLocal<ByteBuffer>> reusableBBHolder = new EnumMap<>(BufferType.class);
+ // Convenience variable holding a ref to the current resuableBB to avoid map lookups
+ private static FastThreadLocal<ByteBuffer> reusableBB;
Review comment:
Ok I see it now. That BB can be of any BBType and as we no longer switch on the fly between types it can't be static.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] bereng commented on a change in pull request #737: CASSANDRA-15880 ThreadLocal removal on close() to prevent leaks
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #737:
URL: https://github.com/apache/cassandra/pull/737#discussion_r494288842
##########
File path: src/java/org/apache/cassandra/db/commitlog/AbstractCommitLogSegmentManager.java
##########
@@ -166,7 +148,22 @@ public void runMayThrow() throws Exception
}
};
+ BufferType BBType = SimpleCachedBufferPool.DEFAULT_PREFERRED_BB_TYPE;
+ if (commitLog.configuration.useEncryption())
+ // Keep reusable buffers on-heap regardless of compression preference so we avoid copy off/on repeatedly during decryption
+ // Also: we want to keep the compression buffers on-heap as we need those bytes for encryption,
+ // and we want to avoid copying from off-heap (compression buffer) to on-heap encryption APIs
+ BBType = BufferType.ON_HEAP;
+ else if (commitLog.configuration.useCompression())
+ BBType = commitLog.configuration.getCompressor().preferredBufferType();
+
+ synchronized(this)
+ {
+ this.bufferPool = new SimpleCachedBufferPool(DatabaseDescriptor.getCommitLogMaxCompressionBuffersInPool(), DatabaseDescriptor.getCommitLogSegmentSize(), BBType);
Review comment:
We can make this more 'solid' by closing the pool before re-creation to defend against `start()` calls without a matching ' close()`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] blerer commented on a change in pull request #737: CASSANDRA-15880 ThreadLocal removal on close() to prevent leaks
Posted by GitBox <gi...@apache.org>.
blerer commented on a change in pull request #737:
URL: https://github.com/apache/cassandra/pull/737#discussion_r490314377
##########
File path: src/java/org/apache/cassandra/db/commitlog/SimpleCachedBufferPool.java
##########
@@ -79,19 +91,20 @@ public ByteBuffer createBuffer(BufferType bufferType)
public ByteBuffer getThreadLocalReusableBuffer(int size)
{
- ByteBuffer result = reusableBufferHolder.get();
- if (result.capacity() < size || BufferType.typeOf(result) != preferredReusableBufferType)
+ ByteBuffer result = reusableBB.get();
+ if (result.capacity() < size)
{
FileUtils.clean(result);
result = preferredReusableBufferType.allocate(size);
- reusableBufferHolder.set(result);
+ reusableBB.set(result);
}
return result;
}
public void setPreferredReusableBufferType(BufferType type)
{
preferredReusableBufferType = type;
+ reusableBB = reusableBBHolder.get(preferredReusableBufferType);
Review comment:
You are right we can still have a race.
The preferedType is set in the `CompressedSegment` and in the `EncryptedSegment` constructor. I do not recall if we can change the commit log configuration without restarting the server. If not, we can figure out which type will be the prefered one based on the `commitLog.configuration`. It is not super clean has it somehow duplicates the logic from `createSegment` but it should work.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] bereng commented on a change in pull request #737: CASSANDRA-15880 ThreadLocal removal on close() to prevent leaks
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #737:
URL: https://github.com/apache/cassandra/pull/737#discussion_r489322822
##########
File path: src/java/org/apache/cassandra/io/util/CompressedChunkReader.java
##########
@@ -85,28 +88,28 @@ public Rebufferer instantiateRebufferer()
public static class Standard extends CompressedChunkReader
{
- // we read the raw compressed bytes into this buffer, then uncompressed them into the provided one.
- private final ThreadLocal<ByteBuffer> compressedHolder;
+ // We read the raw compressed bytes into a buffer, then uncompressed them into the provided one.
+ // Notice we have 1 SimpleCachedBufferPool per BBType which wraps a FastThreadLocal BB
+ private static final EnumMap<BufferType, SimpleCachedBufferPool> reusableCompressBBs = new EnumMap<>(BufferType.class);
Review comment:
:facepalm: apologies...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] bereng commented on pull request #737: CASSANDRA-15880 ThreadLocal removal on close() to prevent leaks
Posted by GitBox <gi...@apache.org>.
bereng commented on pull request #737:
URL: https://github.com/apache/cassandra/pull/737#issuecomment-702578521
Rebased and now failures align more or less to what I see in ci-cass: [J11](https://app.circleci.com/pipelines/github/bereng/cassandra/137/workflows/f7979f36-a277-4f83-beae-ed2dd65a3275) and [J8](https://app.circleci.com/pipelines/github/bereng/cassandra/137/workflows/6ba55bfa-bf69-4ed2-83ed-a4628c0bbcc0)
If we +1 this patch I will squash, push the 3.11 version and run CI both both.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] bereng commented on a change in pull request #737: CASSANDRA-15880 ThreadLocal removal on close() to prevent leaks
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #737:
URL: https://github.com/apache/cassandra/pull/737#discussion_r489509429
##########
File path: src/java/org/apache/cassandra/db/commitlog/SimpleCachedBufferPool.java
##########
@@ -72,6 +74,7 @@ public SimpleCachedBufferPool(int maxBufferPoolSize, int bufferSize)
{
Review comment:
That I tried and then you need to do a `BufferType.typeOf()`... So it's an extra operation vs having that stored in a variable.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org