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 2022/04/25 17:22:49 UTC
[GitHub] [geode] dschneider-pivotal opened a new pull request, #7621: GEODE-10036: remove need to export sun.nio.ch
dschneider-pivotal opened a new pull request, #7621:
URL: https://github.com/apache/geode/pull/7621
BufferPool now returns a PooledByteBuffer instead of ByteBuffer.
The new PooledByteBuffer will keep track of the original ByteBuffer
when the ByteBuffer it exposes is a slice.
This allowed the removal of the DirectBuffer class that accessed
internals and needed sun.nio.ch to be exported.
<!-- 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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on pull request #7621: GEODE-10036: remove need to export sun.nio.ch
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on PR #7621:
URL: https://github.com/apache/geode/pull/7621#issuecomment-1110186561
@pivotal-jbarrett I think this is a trivial change. A radical change would be to use Netty.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a diff in pull request #7621: GEODE-10036: remove need to export sun.nio.ch
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on code in PR #7621:
URL: https://github.com/apache/geode/pull/7621#discussion_r859088287
##########
geode-core/src/main/java/org/apache/geode/internal/net/BufferPool.java:
##########
@@ -90,46 +87,46 @@ static boolean computeUseDirectBuffers() {
*
* @return a byte buffer to be used for sending on this connection.
*/
- public ByteBuffer acquireDirectSenderBuffer(int size) {
+ public PooledByteBuffer acquireDirectSenderBuffer(int size) {
return acquireDirectBuffer(size, true);
}
- public ByteBuffer acquireDirectReceiveBuffer(int size) {
+ public PooledByteBuffer acquireDirectReceiveBuffer(int size) {
return acquireDirectBuffer(size, false);
}
/**
* try to acquire direct buffer, if enabled by configuration
*/
- private ByteBuffer acquireDirectBuffer(int size, boolean send) {
- ByteBuffer result;
+ private PooledByteBuffer acquireDirectBuffer(int size, boolean send) {
+ ByteBuffer byteBuffer;
if (useDirectBuffers) {
if (size <= MEDIUM_BUFFER_SIZE) {
- result = acquirePredefinedFixedBuffer(send, size);
+ byteBuffer = acquirePredefinedFixedBuffer(send, size);
} else {
- result = acquireLargeBuffer(send, size);
+ byteBuffer = acquireLargeBuffer(send, size);
}
- if (result.capacity() > size) {
- result.position(0).limit(size);
- result = result.slice();
+ if (byteBuffer.capacity() > size) {
+ byteBuffer.position(0).limit(size);
+ return new PooledByteBuffer(byteBuffer, byteBuffer.slice());
}
- return result;
+ return new PooledByteBuffer(byteBuffer);
}
// if we are using heap buffers then don't bother with keeping them around
- result = ByteBuffer.allocate(size);
+ byteBuffer = ByteBuffer.allocate(size);
updateBufferStats(size, send, false);
- return result;
+ return new PooledByteBuffer(byteBuffer);
Review Comment:
because we obtains it from the BufferPool. Additional refactoring could be done to not obtain it from BufferPool (the heap buffer are not actually pooled so why do we ask BufferPool for one) but that seemed outside the scope of this PR which was to not export sun.nio.ch
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a diff in pull request #7621: GEODE-10036: remove need to export sun.nio.ch
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on code in PR #7621:
URL: https://github.com/apache/geode/pull/7621#discussion_r859081506
##########
geode-core/src/main/java/org/apache/geode/internal/net/BufferPool.java:
##########
@@ -394,4 +370,36 @@ public ByteBuffer getBB() {
}
}
+ public static class PooledByteBuffer {
+ private final ByteBuffer original;
+ private final ByteBuffer byteBuffer;
Review Comment:
I liked your names and have changed to them. Thanks for the feedback.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] pivotal-jbarrett commented on a diff in pull request #7621: GEODE-10036: remove need to export sun.nio.ch
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on code in PR #7621:
URL: https://github.com/apache/geode/pull/7621#discussion_r859077733
##########
geode-core/src/main/java/org/apache/geode/internal/net/BufferPool.java:
##########
@@ -90,46 +87,46 @@ static boolean computeUseDirectBuffers() {
*
* @return a byte buffer to be used for sending on this connection.
*/
- public ByteBuffer acquireDirectSenderBuffer(int size) {
+ public PooledByteBuffer acquireDirectSenderBuffer(int size) {
return acquireDirectBuffer(size, true);
}
- public ByteBuffer acquireDirectReceiveBuffer(int size) {
+ public PooledByteBuffer acquireDirectReceiveBuffer(int size) {
return acquireDirectBuffer(size, false);
}
/**
* try to acquire direct buffer, if enabled by configuration
*/
- private ByteBuffer acquireDirectBuffer(int size, boolean send) {
- ByteBuffer result;
+ private PooledByteBuffer acquireDirectBuffer(int size, boolean send) {
+ ByteBuffer byteBuffer;
if (useDirectBuffers) {
if (size <= MEDIUM_BUFFER_SIZE) {
- result = acquirePredefinedFixedBuffer(send, size);
+ byteBuffer = acquirePredefinedFixedBuffer(send, size);
} else {
- result = acquireLargeBuffer(send, size);
+ byteBuffer = acquireLargeBuffer(send, size);
}
- if (result.capacity() > size) {
- result.position(0).limit(size);
- result = result.slice();
+ if (byteBuffer.capacity() > size) {
+ byteBuffer.position(0).limit(size);
+ return new PooledByteBuffer(byteBuffer, byteBuffer.slice());
Review Comment:
Going from 1 to 2 transient allocations here.
##########
geode-core/src/main/java/org/apache/geode/internal/net/BufferPool.java:
##########
@@ -90,46 +87,46 @@ static boolean computeUseDirectBuffers() {
*
* @return a byte buffer to be used for sending on this connection.
*/
- public ByteBuffer acquireDirectSenderBuffer(int size) {
+ public PooledByteBuffer acquireDirectSenderBuffer(int size) {
return acquireDirectBuffer(size, true);
}
- public ByteBuffer acquireDirectReceiveBuffer(int size) {
+ public PooledByteBuffer acquireDirectReceiveBuffer(int size) {
return acquireDirectBuffer(size, false);
}
/**
* try to acquire direct buffer, if enabled by configuration
*/
- private ByteBuffer acquireDirectBuffer(int size, boolean send) {
- ByteBuffer result;
+ private PooledByteBuffer acquireDirectBuffer(int size, boolean send) {
+ ByteBuffer byteBuffer;
if (useDirectBuffers) {
if (size <= MEDIUM_BUFFER_SIZE) {
- result = acquirePredefinedFixedBuffer(send, size);
+ byteBuffer = acquirePredefinedFixedBuffer(send, size);
} else {
- result = acquireLargeBuffer(send, size);
+ byteBuffer = acquireLargeBuffer(send, size);
}
- if (result.capacity() > size) {
- result.position(0).limit(size);
- result = result.slice();
+ if (byteBuffer.capacity() > size) {
+ byteBuffer.position(0).limit(size);
+ return new PooledByteBuffer(byteBuffer, byteBuffer.slice());
}
- return result;
+ return new PooledByteBuffer(byteBuffer);
}
// if we are using heap buffers then don't bother with keeping them around
- result = ByteBuffer.allocate(size);
+ byteBuffer = ByteBuffer.allocate(size);
updateBufferStats(size, send, false);
- return result;
+ return new PooledByteBuffer(byteBuffer);
Review Comment:
We don't pool heap buffers, so why the `PooledByteBuffer` wrapper here?
##########
geode-core/src/main/java/org/apache/geode/internal/net/BufferPool.java:
##########
@@ -90,46 +87,46 @@ static boolean computeUseDirectBuffers() {
*
* @return a byte buffer to be used for sending on this connection.
*/
- public ByteBuffer acquireDirectSenderBuffer(int size) {
+ public PooledByteBuffer acquireDirectSenderBuffer(int size) {
return acquireDirectBuffer(size, true);
}
- public ByteBuffer acquireDirectReceiveBuffer(int size) {
+ public PooledByteBuffer acquireDirectReceiveBuffer(int size) {
return acquireDirectBuffer(size, false);
}
/**
* try to acquire direct buffer, if enabled by configuration
*/
- private ByteBuffer acquireDirectBuffer(int size, boolean send) {
- ByteBuffer result;
+ private PooledByteBuffer acquireDirectBuffer(int size, boolean send) {
+ ByteBuffer byteBuffer;
if (useDirectBuffers) {
if (size <= MEDIUM_BUFFER_SIZE) {
- result = acquirePredefinedFixedBuffer(send, size);
+ byteBuffer = acquirePredefinedFixedBuffer(send, size);
} else {
- result = acquireLargeBuffer(send, size);
+ byteBuffer = acquireLargeBuffer(send, size);
}
- if (result.capacity() > size) {
- result.position(0).limit(size);
- result = result.slice();
+ if (byteBuffer.capacity() > size) {
+ byteBuffer.position(0).limit(size);
+ return new PooledByteBuffer(byteBuffer, byteBuffer.slice());
}
- return result;
+ return new PooledByteBuffer(byteBuffer);
Review Comment:
Going from 0 to 1 transient allocations here.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on pull request #7621: GEODE-10036: remove need to export sun.nio.ch
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on PR #7621:
URL: https://github.com/apache/geode/pull/7621#issuecomment-1110194657
@pivotal-jbarrett I also was concerned about the code smell that in the past we didn't just use limit instead of capacity. But given that someone went to the trouble of accessing a jdk internal feature to solve that issue, it seemed safer to not try to find the places that depended on capacity and limit being equal. We could do that and then might not beed the PooledByteBuffer instances. Another option would be to maintain a map in BufferPool that allows a slice being returned to the pool to have its original instance looked up. Then the ones that pay the extra overhead are when a slice was taken.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] pivotal-jbarrett commented on pull request #7621: GEODE-10036: remove need to export sun.nio.ch
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on PR #7621:
URL: https://github.com/apache/geode/pull/7621#issuecomment-1110170895
@dschneider-pivotal I believe the original intent on using the hidden direct buffer method was to avoid the allocation of yet another transient object. More transient allocations in the hot path of p2p communication could have negative performance impacts. Honestly the fact that we had to return a specifically sized slice back from the pool was a concerning smell to me. Perhaps there is a way to solve this by just returning the actual buffer with just the limit set.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on pull request #7621: GEODE-10036: remove need to export sun.nio.ch
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on PR #7621:
URL: https://github.com/apache/geode/pull/7621#issuecomment-1111270762
We decided not to make this change for 1.15 since this part of geode will function in 1.17 with the addition of an "add-export" of sun.nio.ch. In a future release we might make a bigger change to how geode pools direct buffers.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] Bill commented on a diff in pull request #7621: GEODE-10036: remove need to export sun.nio.ch
Posted by GitBox <gi...@apache.org>.
Bill commented on code in PR #7621:
URL: https://github.com/apache/geode/pull/7621#discussion_r858988486
##########
geode-core/src/main/java/org/apache/geode/internal/net/BufferPool.java:
##########
@@ -394,4 +370,36 @@ public ByteBuffer getBB() {
}
}
+ public static class PooledByteBuffer {
Review Comment:
At first I was gonna ask you to put `PooledByteBuffer` in it's own file, but now that I see it's so tiny, I think it's ok as is.
##########
geode-core/src/main/java/org/apache/geode/internal/net/BufferPool.java:
##########
@@ -394,4 +370,36 @@ public ByteBuffer getBB() {
}
}
+ public static class PooledByteBuffer {
+ private final ByteBuffer original;
+ private final ByteBuffer byteBuffer;
Review Comment:
These field names are ok but they would make more sense to me as `poolableBuffer` and `usableSlice`.
**But I will approve this PR as-is if you don't like this suggestion.**
If you like this idea you might continue to call the accessor returning `slice`: `getBuffer()`. The mismatch is just fine since the field name is an internal detail, the accessor is for users of the object. Most users of this class (besides `BufferPool`) don't care that they are getting a slice.
I'd change `getByteBuffer()` to be `getPoolableBuffer()`.
##########
geode-core/src/main/java/org/apache/geode/internal/net/BufferPool.java:
##########
@@ -394,4 +370,36 @@ public ByteBuffer getBB() {
}
}
+ public static class PooledByteBuffer {
+ private final ByteBuffer original;
+ private final ByteBuffer byteBuffer;
+
+ PooledByteBuffer(ByteBuffer original, ByteBuffer byteBuffer) {
+ this.original = original;
+ this.byteBuffer = byteBuffer;
+ }
+
+ PooledByteBuffer(ByteBuffer byteBuffer) {
+ this(byteBuffer, byteBuffer);
+ }
+
+ /**
+ * Returns the byte buffer intended for use outside of the pool.
+ */
+ public ByteBuffer getByteBuffer() {
+ return byteBuffer;
+ }
+
+ /**
+ * The original byte buffer managed by the pool.
+ * This should only be used by the pool itself.
+ * It may differ from 'byteBuffer' since it may be a slice of the original.
Review Comment:
Please clarify the second sentence. The two "it"s in this sentence refer to different things so the sentence is hard to understand. The first "it" refers to `original`. The sentence only makes sense if the second "it" refers to `byteBuffer`.
We might say:
"'byteBuffer' may be a slice of 'original'."
##########
geode-core/src/main/java/org/apache/geode/internal/net/BufferPool.java:
##########
@@ -394,4 +370,36 @@ public ByteBuffer getBB() {
}
}
+ public static class PooledByteBuffer {
+ private final ByteBuffer original;
+ private final ByteBuffer byteBuffer;
+
+ PooledByteBuffer(ByteBuffer original, ByteBuffer byteBuffer) {
Review Comment:
Good. I verified the only place this is being called outside the class is in the one place the pool slices a buffer before returning it.
##########
geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/MemberJvmOptions.java:
##########
@@ -29,7 +29,6 @@
public class MemberJvmOptions {
static final int CMS_INITIAL_OCCUPANCY_FRACTION = 60;
private static final List<String> JAVA_11_OPTIONS = Arrays.asList(
- "--add-exports=java.base/sun.nio.ch=ALL-UNNAMED",
Review Comment:
Yay!
##########
geode-gfsh/src/test/java/org/apache/geode/management/internal/cli/commands/MemberJvmOptionsTest.java:
##########
@@ -43,7 +43,6 @@ void java8Options() {
@EnabledForJreRange(min = JRE.JAVA_11)
void java11Options() {
List<String> expectedOptions = asList(
- "--add-exports=java.base/sun.nio.ch=ALL-UNNAMED",
Review Comment:
🐳 done! (well done)
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal closed pull request #7621: GEODE-10036: remove need to export sun.nio.ch
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal closed pull request #7621: GEODE-10036: remove need to export sun.nio.ch
URL: https://github.com/apache/geode/pull/7621
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a diff in pull request #7621: GEODE-10036: remove need to export sun.nio.ch
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on code in PR #7621:
URL: https://github.com/apache/geode/pull/7621#discussion_r859086223
##########
geode-core/src/main/java/org/apache/geode/internal/net/BufferPool.java:
##########
@@ -90,46 +87,46 @@ static boolean computeUseDirectBuffers() {
*
* @return a byte buffer to be used for sending on this connection.
*/
- public ByteBuffer acquireDirectSenderBuffer(int size) {
+ public PooledByteBuffer acquireDirectSenderBuffer(int size) {
return acquireDirectBuffer(size, true);
}
- public ByteBuffer acquireDirectReceiveBuffer(int size) {
+ public PooledByteBuffer acquireDirectReceiveBuffer(int size) {
return acquireDirectBuffer(size, false);
}
/**
* try to acquire direct buffer, if enabled by configuration
*/
- private ByteBuffer acquireDirectBuffer(int size, boolean send) {
- ByteBuffer result;
+ private PooledByteBuffer acquireDirectBuffer(int size, boolean send) {
+ ByteBuffer byteBuffer;
if (useDirectBuffers) {
if (size <= MEDIUM_BUFFER_SIZE) {
- result = acquirePredefinedFixedBuffer(send, size);
+ byteBuffer = acquirePredefinedFixedBuffer(send, size);
} else {
- result = acquireLargeBuffer(send, size);
+ byteBuffer = acquireLargeBuffer(send, size);
}
- if (result.capacity() > size) {
- result.position(0).limit(size);
- result = result.slice();
+ if (byteBuffer.capacity() > size) {
+ byteBuffer.position(0).limit(size);
+ return new PooledByteBuffer(byteBuffer, byteBuffer.slice());
Review Comment:
I think these transient allocations are a drop in the bucket compared to the total number of transient allocations we have for each cache operation. And these transient instances are very small. I don't think it was worth using an internal JDK feature which is subject to change in any jdk release to avoid this transient.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a diff in pull request #7621: GEODE-10036: remove need to export sun.nio.ch
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on code in PR #7621:
URL: https://github.com/apache/geode/pull/7621#discussion_r859090971
##########
geode-core/src/main/java/org/apache/geode/internal/net/BufferPool.java:
##########
@@ -90,46 +87,46 @@ static boolean computeUseDirectBuffers() {
*
* @return a byte buffer to be used for sending on this connection.
*/
- public ByteBuffer acquireDirectSenderBuffer(int size) {
+ public PooledByteBuffer acquireDirectSenderBuffer(int size) {
return acquireDirectBuffer(size, true);
}
- public ByteBuffer acquireDirectReceiveBuffer(int size) {
+ public PooledByteBuffer acquireDirectReceiveBuffer(int size) {
return acquireDirectBuffer(size, false);
}
/**
* try to acquire direct buffer, if enabled by configuration
*/
- private ByteBuffer acquireDirectBuffer(int size, boolean send) {
- ByteBuffer result;
+ private PooledByteBuffer acquireDirectBuffer(int size, boolean send) {
+ ByteBuffer byteBuffer;
if (useDirectBuffers) {
if (size <= MEDIUM_BUFFER_SIZE) {
- result = acquirePredefinedFixedBuffer(send, size);
+ byteBuffer = acquirePredefinedFixedBuffer(send, size);
} else {
- result = acquireLargeBuffer(send, size);
+ byteBuffer = acquireLargeBuffer(send, size);
}
- if (result.capacity() > size) {
- result.position(0).limit(size);
- result = result.slice();
+ if (byteBuffer.capacity() > size) {
+ byteBuffer.position(0).limit(size);
+ return new PooledByteBuffer(byteBuffer, byteBuffer.slice());
Review Comment:
Also keep in mind that half of the users of BufferPool get a buffer and hold onto for an extended time. In those cases the PooledByteBuffer is not transient.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] pivotal-jbarrett commented on a diff in pull request #7621: GEODE-10036: remove need to export sun.nio.ch
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on code in PR #7621:
URL: https://github.com/apache/geode/pull/7621#discussion_r859096798
##########
geode-core/src/main/java/org/apache/geode/internal/net/BufferPool.java:
##########
@@ -90,46 +87,46 @@ static boolean computeUseDirectBuffers() {
*
* @return a byte buffer to be used for sending on this connection.
*/
- public ByteBuffer acquireDirectSenderBuffer(int size) {
+ public PooledByteBuffer acquireDirectSenderBuffer(int size) {
return acquireDirectBuffer(size, true);
}
- public ByteBuffer acquireDirectReceiveBuffer(int size) {
+ public PooledByteBuffer acquireDirectReceiveBuffer(int size) {
return acquireDirectBuffer(size, false);
}
/**
* try to acquire direct buffer, if enabled by configuration
*/
- private ByteBuffer acquireDirectBuffer(int size, boolean send) {
- ByteBuffer result;
+ private PooledByteBuffer acquireDirectBuffer(int size, boolean send) {
+ ByteBuffer byteBuffer;
if (useDirectBuffers) {
if (size <= MEDIUM_BUFFER_SIZE) {
- result = acquirePredefinedFixedBuffer(send, size);
+ byteBuffer = acquirePredefinedFixedBuffer(send, size);
} else {
- result = acquireLargeBuffer(send, size);
+ byteBuffer = acquireLargeBuffer(send, size);
}
- if (result.capacity() > size) {
- result.position(0).limit(size);
- result = result.slice();
+ if (byteBuffer.capacity() > size) {
+ byteBuffer.position(0).limit(size);
+ return new PooledByteBuffer(byteBuffer, byteBuffer.slice());
Review Comment:
The other half that don't invoke this method rapidly to get send buffers. While the size is small the effect of number of objects can be detrimental and if everyone along the path says it's small in the larger scope we eventually end up with noticeable latency. Great care has go into removing allocations like this in hot paths like this because they had measurable impact. While I agree p2p has many worse places for transient allocations I don't see it as proof to support a new one. I think we can avoid this completely by just returning the original `ByteBuffer`. To do so we need to stop slicing, which is not 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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] pivotal-jbarrett commented on pull request #7621: GEODE-10036: remove need to export sun.nio.ch
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on PR #7621:
URL: https://github.com/apache/geode/pull/7621#issuecomment-1110177147
If we are going to make radical changes to our `ByteBuffer` pooling I strongly suggest we just replace it with Netty's `ByteBuf` pooling.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org