[
https://issues.apache.org/jira/browse/GEODE-8506?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17198066#comment-17198066
]
ASF GitHub Bot commented on GEODE-8506:
---------------------------------------
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:
us...@infra.apache.org
> BufferPool returns byte buffers that may be much larger than requested
> ----------------------------------------------------------------------
>
> Key: GEODE-8506
> URL: https://issues.apache.org/jira/browse/GEODE-8506
> Project: Geode
> Issue Type: Improvement
> Components: membership
> Reporter: Bruce J Schuchardt
> Assignee: Bruce J Schuchardt
> Priority: Major
> Labels: pull-request-available
>
> BufferPool manages several pools of direct-memory ByteBuffers. When asked
> for a ByteBuffer of size X you may receive a buffer that is any size greater
> than or equal to X. For users of this pool this is unexpected behavior and
> is causing some trouble.
> MessageStreamer, for instance, performs message "chunking" based on the size
> of a socket's buffer size. It requests a byte buffer of that size and then
> fills it over and over again with message chunks to be written to the socket.
> But it does this based on the buffer's capacity, which may be much larger
> than the expected buffer size. This results in incorrect chunking and
> requires larger buffers in the receiver of these message chunks.
> BufferPool should always return a buffer that has exactly the requested
> capacity. It could be a _slice_ of a pooled buffer, for instance. That
> would let it hand out a larger buffer while not confusing the code that
> requested the buffer.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
