You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2020/09/17 23:10:24 UTC
[GitHub] [geode] bschuchardt opened a new pull request #5525: GEODE-8506: BufferPool returns byte buffers that may be much larger t…
bschuchardt opened a new pull request #5525:
URL: https://github.com/apache/geode/pull/5525
…han requested
Create a "slice" of the acquired buffer to return to the caller instead
of returning a buffer larger than what was requested. On return we fish
out the parent buffer and put it back in the pool.
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [ ] Is your initial contribution a single, squashed commit?
- [ ] Does `gradlew build` run cleanly?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
### Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
----------------------------------------------------------------
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
[GitHub] [geode] bschuchardt commented on a change in pull request #5525: GEODE-8506: BufferPool returns byte buffers that may be much larger t…
Posted by GitBox <gi...@apache.org>.
bschuchardt commented on a change in pull request #5525:
URL: https://github.com/apache/geode/pull/5525#discussion_r491016215
##########
File path: geode-core/src/main/java/org/apache/geode/internal/net/BufferPool.java
##########
@@ -98,14 +101,18 @@ private ByteBuffer acquireDirectBuffer(int size, boolean send) {
if (useDirectBuffers) {
if (size <= MEDIUM_BUFFER_SIZE) {
- return acquirePredefinedFixedBuffer(send, size);
+ result = acquirePredefinedFixedBuffer(send, size);
Review comment:
I didn't change that private method and AFAIK the comment is still accurate
----------------------------------------------------------------
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
[GitHub] [geode] Bill commented on a change in pull request #5525: GEODE-8506: BufferPool returns byte buffers that may be much larger t…
Posted by GitBox <gi...@apache.org>.
Bill commented on a change in pull request #5525:
URL: https://github.com/apache/geode/pull/5525#discussion_r490654223
##########
File path: geode-core/src/main/java/org/apache/geode/internal/net/BufferPool.java
##########
@@ -98,14 +101,18 @@ private ByteBuffer acquireDirectBuffer(int size, boolean send) {
if (useDirectBuffers) {
if (size <= MEDIUM_BUFFER_SIZE) {
- return acquirePredefinedFixedBuffer(send, size);
+ result = acquirePredefinedFixedBuffer(send, size);
Review comment:
The comment on method `acquirePredefinedFixedBuffer()` now lies. It says:
```
/**
* Acquire direct buffer with predefined default capacity (4096 or 32768)
*/
```
But that method no longer returns buffers with predefined _capacity_.
~Also that method's name seems wrong since it doesn't return a buffer with a predefined capacity at all, rather, it returns a buffer of the specified `size`.~ brainf*rt
If I am understanding the method in question, perhaps this would be more accurate (also eliminates the word "fixed" from the method name since I don't think that is pertinent):
```
/**
* Acquire direct buffer of size <= MEDIUM_BUFFER_SIZE
*
* Caller ensures that size <= MEDIUM_BUFFER_SIZE on entry. This method
* will not check that!
*/
private ByteBuffer acquireSmallToMediumBuffer(boolean send, int size) {
```
----------------------------------------------------------------
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
[GitHub] [geode] bschuchardt commented on a change in pull request #5525: GEODE-8506: BufferPool returns byte buffers that may be much larger t…
Posted by GitBox <gi...@apache.org>.
bschuchardt commented on a change in pull request #5525:
URL: https://github.com/apache/geode/pull/5525#discussion_r491013014
##########
File path: geode-core/src/main/java/org/apache/geode/internal/net/BufferPool.java
##########
@@ -295,19 +302,48 @@ void releaseBuffer(BufferPool.BufferType type, ByteBuffer buffer) {
/**
* Releases a previously acquired buffer.
*/
- private void releaseBuffer(ByteBuffer bb, boolean send) {
- if (bb.isDirect()) {
- BBSoftReference bbRef = new BBSoftReference(bb, send);
- if (bb.capacity() <= SMALL_BUFFER_SIZE) {
+ private void releaseBuffer(ByteBuffer buffer, boolean send) {
+ if (buffer.isDirect()) {
+ buffer = getPoolableBuffer(buffer);
+ BBSoftReference bbRef = new BBSoftReference(buffer, send);
+ if (buffer.capacity() <= SMALL_BUFFER_SIZE) {
bufferSmallQueue.offer(bbRef);
- } else if (bb.capacity() <= MEDIUM_BUFFER_SIZE) {
+ } else if (buffer.capacity() <= MEDIUM_BUFFER_SIZE) {
bufferMiddleQueue.offer(bbRef);
} else {
bufferLargeQueue.offer(bbRef);
}
} else {
- updateBufferStats(-bb.capacity(), send, false);
+ updateBufferStats(-buffer.capacity(), send, false);
+ }
+ }
+
+ /**
+ * If we hand out a buffer that is larger than the requested size we create a
+ * "slice" of the buffer having the requested capacity and hand that out instead.
+ * When we put the buffer back in the pool we need to find the original, non-sliced,
+ * buffer. This is held in DirectBuffer in its "attachment" field, which is a public
+ * method, though DirectBuffer is package-private.
+ */
+ @VisibleForTesting
+ public ByteBuffer getPoolableBuffer(ByteBuffer buffer) {
Review comment:
That seems like an expensive change that would disturb a lot of code.
----------------------------------------------------------------
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
[GitHub] [geode] Bill commented on a change in pull request #5525: GEODE-8506: BufferPool returns byte buffers that may be much larger t…
Posted by GitBox <gi...@apache.org>.
Bill commented on a change in pull request #5525:
URL: https://github.com/apache/geode/pull/5525#discussion_r491066133
##########
File path: geode-core/src/main/java/org/apache/geode/internal/net/BufferPool.java
##########
@@ -98,14 +101,18 @@ private ByteBuffer acquireDirectBuffer(int size, boolean send) {
if (useDirectBuffers) {
if (size <= MEDIUM_BUFFER_SIZE) {
- return acquirePredefinedFixedBuffer(send, size);
+ result = acquirePredefinedFixedBuffer(send, size);
Review comment:
I'm retracting that comment since this method name existed before this PR and was arguably already wrong before this PR.
Any such changes should be pursued in connection with a refactoring ticket.
----------------------------------------------------------------
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
[GitHub] [geode] bschuchardt commented on a change in pull request #5525: GEODE-8506: BufferPool returns byte buffers that may be much larger t…
Posted by GitBox <gi...@apache.org>.
bschuchardt commented on a change in pull request #5525:
URL: https://github.com/apache/geode/pull/5525#discussion_r491044218
##########
File path: geode-core/src/main/java/org/apache/geode/internal/net/BufferPool.java
##########
@@ -98,14 +101,18 @@ private ByteBuffer acquireDirectBuffer(int size, boolean send) {
if (useDirectBuffers) {
if (size <= MEDIUM_BUFFER_SIZE) {
- return acquirePredefinedFixedBuffer(send, size);
+ result = acquirePredefinedFixedBuffer(send, size);
Review comment:
I would prefer to just remove the magic numbers from the comment. I don't think the method rename is warranted.
----------------------------------------------------------------
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
[GitHub] [geode] Bill commented on a change in pull request #5525: GEODE-8506: BufferPool returns byte buffers that may be much larger t…
Posted by GitBox <gi...@apache.org>.
Bill commented on a change in pull request #5525:
URL: https://github.com/apache/geode/pull/5525#discussion_r491018765
##########
File path: geode-core/src/main/java/org/apache/geode/internal/net/BufferPool.java
##########
@@ -295,19 +302,48 @@ void releaseBuffer(BufferPool.BufferType type, ByteBuffer buffer) {
/**
* Releases a previously acquired buffer.
*/
- private void releaseBuffer(ByteBuffer bb, boolean send) {
- if (bb.isDirect()) {
- BBSoftReference bbRef = new BBSoftReference(bb, send);
- if (bb.capacity() <= SMALL_BUFFER_SIZE) {
+ private void releaseBuffer(ByteBuffer buffer, boolean send) {
+ if (buffer.isDirect()) {
+ buffer = getPoolableBuffer(buffer);
+ BBSoftReference bbRef = new BBSoftReference(buffer, send);
+ if (buffer.capacity() <= SMALL_BUFFER_SIZE) {
bufferSmallQueue.offer(bbRef);
- } else if (bb.capacity() <= MEDIUM_BUFFER_SIZE) {
+ } else if (buffer.capacity() <= MEDIUM_BUFFER_SIZE) {
bufferMiddleQueue.offer(bbRef);
} else {
bufferLargeQueue.offer(bbRef);
}
} else {
- updateBufferStats(-bb.capacity(), send, false);
+ updateBufferStats(-buffer.capacity(), send, false);
+ }
+ }
+
+ /**
+ * If we hand out a buffer that is larger than the requested size we create a
+ * "slice" of the buffer having the requested capacity and hand that out instead.
+ * When we put the buffer back in the pool we need to find the original, non-sliced,
+ * buffer. This is held in DirectBuffer in its "attachment" field, which is a public
+ * method, though DirectBuffer is package-private.
+ */
+ @VisibleForTesting
+ public ByteBuffer getPoolableBuffer(ByteBuffer buffer) {
Review comment:
That’s fine. But I did want to run it by you.
Subclassing from a byte buffer class would entail making large (lots of methods), new classes huh?
The alternative would be to return something that is not a byte buffer, but rather has-a byte buffer. In that case things that call our `BufferPool` methods would get one of those (non-byte-buffer) things, and that would perturb lots of calling code.
----------------------------------------------------------------
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
[GitHub] [geode] echobravopapa commented on a change in pull request #5525: GEODE-8506: BufferPool returns byte buffers that may be much larger t…
Posted by GitBox <gi...@apache.org>.
echobravopapa commented on a change in pull request #5525:
URL: https://github.com/apache/geode/pull/5525#discussion_r490616848
##########
File path: geode-core/src/main/java/org/apache/geode/internal/net/BufferPool.java
##########
@@ -98,14 +101,18 @@ private ByteBuffer acquireDirectBuffer(int size, boolean send) {
if (useDirectBuffers) {
if (size <= MEDIUM_BUFFER_SIZE) {
- return acquirePredefinedFixedBuffer(send, size);
+ result = acquirePredefinedFixedBuffer(send, size);
} else {
- return acquireLargeBuffer(send, size);
+ result = acquireLargeBuffer(send, size);
}
- } else {
- // if we are using heap buffers then don't bother with keeping them around
- result = ByteBuffer.allocate(size);
+ if (result.capacity() > size) {
+ result.position(0).limit(size);
+ result = result.slice();
Review comment:
👍
----------------------------------------------------------------
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
[GitHub] [geode] Bill commented on a change in pull request #5525: GEODE-8506: BufferPool returns byte buffers that may be much larger t…
Posted by GitBox <gi...@apache.org>.
Bill commented on a change in pull request #5525:
URL: https://github.com/apache/geode/pull/5525#discussion_r490648118
##########
File path: geode-core/src/test/java/org/apache/geode/internal/net/BufferPoolTest.java
##########
@@ -133,27 +135,42 @@ public void checkBufferSizeAfterAllocation() throws Exception {
@Test
public void checkBufferSizeAfterAcquire() throws Exception {
+ // allocate a small buffer and a larger buffer. Check their sizes, etc and then
+ // release and reacquire them. They should be from separate buffer pools so there
+ // should still be a small buffer and a larger buffer.
Review comment:
These should be called something like `smallBuffer` and `largerBuffer` or something, based on this comment.
##########
File path: geode-core/src/main/java/org/apache/geode/internal/net/BufferPool.java
##########
@@ -98,14 +101,18 @@ private ByteBuffer acquireDirectBuffer(int size, boolean send) {
if (useDirectBuffers) {
if (size <= MEDIUM_BUFFER_SIZE) {
- return acquirePredefinedFixedBuffer(send, size);
+ result = acquirePredefinedFixedBuffer(send, size);
Review comment:
The comment on method `acquirePredefinedFixedBuffer()` now lies. It says:
```
/**
* Acquire direct buffer with predefined default capacity (4096 or 32768)
*/
```
But that method no longer returns buffers with predefined _capacity_.
Also that method's name seems wrong since it doesn't return a buffer with a predefined capacity at all, rather, it returns a buffer of the specified `size`.
If I am understanding the method in question, perhaps this would be more accurate:
```
/**
* Acquire direct buffer of size <= MEDIUM_BUFFER_SIZE
*
* Caller ensures that size <= MEDIUM_BUFFER_SIZE on entry. This method
* will not check that!
*/
private ByteBuffer acquireSmallToMediumBuffer(boolean send, int size) {
```
##########
File path: geode-core/src/test/java/org/apache/geode/internal/net/BufferPoolTest.java
##########
@@ -133,27 +135,42 @@ public void checkBufferSizeAfterAllocation() throws Exception {
@Test
public void checkBufferSizeAfterAcquire() throws Exception {
+ // allocate a small buffer and a larger buffer. Check their sizes, etc and then
+ // release and reacquire them. They should be from separate buffer pools so there
+ // should still be a small buffer and a larger buffer.
ByteBuffer buffer = bufferPool.acquireDirectReceiveBuffer(100);
ByteBuffer newBuffer =
bufferPool.acquireDirectReceiveBuffer(10000);
- assertThat(buffer.capacity()).isGreaterThanOrEqualTo(4096);
- assertThat(newBuffer.capacity()).isGreaterThanOrEqualTo(32768);
+ assertThat(buffer.capacity()).isEqualTo(100);
+ assertThat(newBuffer.capacity()).isEqualTo(10000);
+ assertThat(buffer.isDirect()).isTrue();
+ assertThat(newBuffer.isDirect()).isTrue();
+ assertThat(bufferPool.getPoolableBuffer(buffer).capacity())
+ .isGreaterThanOrEqualTo(4096);
Review comment:
This test would make more sense if I knew where this magic number, `4096`, came from. Recommend either referencing the pertinent constant directly or at least including a comment here.
Same comment for `32768` on next line…
##########
File path: geode-core/src/main/java/org/apache/geode/internal/net/BufferPool.java
##########
@@ -295,19 +302,48 @@ void releaseBuffer(BufferPool.BufferType type, ByteBuffer buffer) {
/**
* Releases a previously acquired buffer.
*/
- private void releaseBuffer(ByteBuffer bb, boolean send) {
- if (bb.isDirect()) {
- BBSoftReference bbRef = new BBSoftReference(bb, send);
- if (bb.capacity() <= SMALL_BUFFER_SIZE) {
+ private void releaseBuffer(ByteBuffer buffer, boolean send) {
+ if (buffer.isDirect()) {
+ buffer = getPoolableBuffer(buffer);
+ BBSoftReference bbRef = new BBSoftReference(buffer, send);
+ if (buffer.capacity() <= SMALL_BUFFER_SIZE) {
bufferSmallQueue.offer(bbRef);
- } else if (bb.capacity() <= MEDIUM_BUFFER_SIZE) {
+ } else if (buffer.capacity() <= MEDIUM_BUFFER_SIZE) {
bufferMiddleQueue.offer(bbRef);
} else {
bufferLargeQueue.offer(bbRef);
}
} else {
- updateBufferStats(-bb.capacity(), send, false);
+ updateBufferStats(-buffer.capacity(), send, false);
+ }
+ }
+
+ /**
+ * If we hand out a buffer that is larger than the requested size we create a
+ * "slice" of the buffer having the requested capacity and hand that out instead.
+ * When we put the buffer back in the pool we need to find the original, non-sliced,
+ * buffer. This is held in DirectBuffer in its "attachment" field, which is a public
+ * method, though DirectBuffer is package-private.
+ */
+ @VisibleForTesting
+ public ByteBuffer getPoolableBuffer(ByteBuffer buffer) {
Review comment:
It is awful that this method has to reflect on every single returned `ByteBuffer`.
Would it impose too much overhead for us to define our own hierarchy underneath that abstract class to do the work (polymorphically) of returning the buffer to the pool? I realize that would entail three classes. But I wonder if they would be small (in lines-of-code). That would eliminate the possibility of an `InternalGemFireException` here and that would be valuable.
----------------------------------------------------------------
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
[GitHub] [geode] Bill commented on a change in pull request #5525: GEODE-8506: BufferPool returns byte buffers that may be much larger t…
Posted by GitBox <gi...@apache.org>.
Bill commented on a change in pull request #5525:
URL: https://github.com/apache/geode/pull/5525#discussion_r491065723
##########
File path: geode-core/src/test/java/org/apache/geode/internal/net/BufferPoolTest.java
##########
@@ -133,27 +135,42 @@ public void checkBufferSizeAfterAllocation() throws Exception {
@Test
public void checkBufferSizeAfterAcquire() throws Exception {
+ // allocate a small buffer and a larger buffer. Check their sizes, etc and then
+ // release and reacquire them. They should be from separate buffer pools so there
+ // should still be a small buffer and a larger buffer.
ByteBuffer buffer = bufferPool.acquireDirectReceiveBuffer(100);
ByteBuffer newBuffer =
bufferPool.acquireDirectReceiveBuffer(10000);
- assertThat(buffer.capacity()).isGreaterThanOrEqualTo(4096);
- assertThat(newBuffer.capacity()).isGreaterThanOrEqualTo(32768);
+ assertThat(buffer.capacity()).isEqualTo(100);
+ assertThat(newBuffer.capacity()).isEqualTo(10000);
+ assertThat(buffer.isDirect()).isTrue();
+ assertThat(newBuffer.isDirect()).isTrue();
+ assertThat(bufferPool.getPoolableBuffer(buffer).capacity())
+ .isGreaterThanOrEqualTo(4096);
Review comment:
I'm retracting that comment since these constants existed before this PR.
Any such changes should be pursued in connection with a refactoring ticket.
----------------------------------------------------------------
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
[GitHub] [geode] bschuchardt commented on a change in pull request #5525: GEODE-8506: BufferPool returns byte buffers that may be much larger t…
Posted by GitBox <gi...@apache.org>.
bschuchardt commented on a change in pull request #5525:
URL: https://github.com/apache/geode/pull/5525#discussion_r491045634
##########
File path: geode-core/src/main/java/org/apache/geode/internal/net/BufferPool.java
##########
@@ -295,19 +302,48 @@ void releaseBuffer(BufferPool.BufferType type, ByteBuffer buffer) {
/**
* Releases a previously acquired buffer.
*/
- private void releaseBuffer(ByteBuffer bb, boolean send) {
- if (bb.isDirect()) {
- BBSoftReference bbRef = new BBSoftReference(bb, send);
- if (bb.capacity() <= SMALL_BUFFER_SIZE) {
+ private void releaseBuffer(ByteBuffer buffer, boolean send) {
+ if (buffer.isDirect()) {
+ buffer = getPoolableBuffer(buffer);
+ BBSoftReference bbRef = new BBSoftReference(buffer, send);
+ if (buffer.capacity() <= SMALL_BUFFER_SIZE) {
bufferSmallQueue.offer(bbRef);
- } else if (bb.capacity() <= MEDIUM_BUFFER_SIZE) {
+ } else if (buffer.capacity() <= MEDIUM_BUFFER_SIZE) {
bufferMiddleQueue.offer(bbRef);
} else {
bufferLargeQueue.offer(bbRef);
}
} else {
- updateBufferStats(-bb.capacity(), send, false);
+ updateBufferStats(-buffer.capacity(), send, false);
+ }
+ }
+
+ /**
+ * If we hand out a buffer that is larger than the requested size we create a
+ * "slice" of the buffer having the requested capacity and hand that out instead.
+ * When we put the buffer back in the pool we need to find the original, non-sliced,
+ * buffer. This is held in DirectBuffer in its "attachment" field, which is a public
+ * method, though DirectBuffer is package-private.
+ */
+ @VisibleForTesting
+ public ByteBuffer getPoolableBuffer(ByteBuffer buffer) {
Review comment:
Subclassing ByteBuffer isn't possible due to the class having final methods. Introducing a new class seems desirable but would affect a lot of code and this needs to be backported to 1.12 and 1.13 support branches. In such cases it's best to keep code changes to a minimum.
----------------------------------------------------------------
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
[GitHub] [geode] Bill commented on a change in pull request #5525: GEODE-8506: BufferPool returns byte buffers that may be much larger t…
Posted by GitBox <gi...@apache.org>.
Bill commented on a change in pull request #5525:
URL: https://github.com/apache/geode/pull/5525#discussion_r491065581
##########
File path: geode-core/src/test/java/org/apache/geode/internal/net/BufferPoolTest.java
##########
@@ -133,27 +135,42 @@ public void checkBufferSizeAfterAllocation() throws Exception {
@Test
public void checkBufferSizeAfterAcquire() throws Exception {
+ // allocate a small buffer and a larger buffer. Check their sizes, etc and then
+ // release and reacquire them. They should be from separate buffer pools so there
+ // should still be a small buffer and a larger buffer.
Review comment:
I'm retracting that comment since these variable names existed before this PR.
Any such changes should be pursued in connection with a refactoring ticket.
----------------------------------------------------------------
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
[GitHub] [geode] bschuchardt merged pull request #5525: GEODE-8506: BufferPool returns byte buffers that may be much larger t…
Posted by GitBox <gi...@apache.org>.
bschuchardt merged pull request #5525:
URL: https://github.com/apache/geode/pull/5525
----------------------------------------------------------------
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