stefanvodita commented on code in PR #12506:
URL: https://github.com/apache/lucene/pull/12506#discussion_r1375044479
##########
lucene/core/src/java/org/apache/lucene/util/ByteBlockPool.java:
##########
@@ -170,42 +191,42 @@ public void reset(boolean zeroFillBuffers, boolean
reuseFirst) {
}
/**
- * Advances the pool to its next buffer. This method should be called once
after the constructor
- * to initialize the pool. In contrast to the constructor a {@link
ByteBlockPool#reset()} call
- * will advance the pool to its first buffer immediately.
+ * Allocates a new buffer and advances the pool to it. This method should be
called once after the
+ * constructor to initialize the pool. In contrast to the constructor, a
{@link
+ * ByteBlockPool#reset()} call will advance the pool to its first buffer
immediately.
*/
public void nextBuffer() {
if (1 + bufferUpto == buffers.length) {
+ // The buffer array is full - expand it
byte[][] newBuffers =
new byte[ArrayUtil.oversize(buffers.length + 1,
NUM_BYTES_OBJECT_REF)][];
System.arraycopy(buffers, 0, newBuffers, 0, buffers.length);
buffers = newBuffers;
}
+ // Allocate new buffer and advance the pool to it
buffer = buffers[1 + bufferUpto] = allocator.getByteBlock();
bufferUpto++;
-
byteUpto = 0;
byteOffset = Math.addExact(byteOffset, BYTE_BLOCK_SIZE);
}
/**
- * Fill the provided {@link BytesRef} with the bytes at the specified
offset/length slice. This
- * will avoid copying the bytes, if the slice fits into a single block;
otherwise, it uses the
- * provided {@link BytesRefBuilder} to copy bytes over.
+ * Fill the provided {@link BytesRef} with the bytes at the specified offset
and length. This will
+ * avoid copying the bytes if the slice fits into a single block; otherwise,
it uses the provided
+ * {@link BytesRefBuilder} to copy bytes over.
*/
- void setBytesRef(BytesRefBuilder builder, BytesRef result, long offset, int
length) {
+ void setBytesRef(BytesRefBuilder builder, BytesRef result, int offset, int
length) {
Review Comment:
I missed that the purpose of the long was to increase the address space.
I'll change this back.
##########
lucene/core/src/java/org/apache/lucene/util/ByteBlockPool.java:
##########
@@ -170,42 +191,42 @@ public void reset(boolean zeroFillBuffers, boolean
reuseFirst) {
}
/**
- * Advances the pool to its next buffer. This method should be called once
after the constructor
- * to initialize the pool. In contrast to the constructor a {@link
ByteBlockPool#reset()} call
- * will advance the pool to its first buffer immediately.
+ * Allocates a new buffer and advances the pool to it. This method should be
called once after the
+ * constructor to initialize the pool. In contrast to the constructor, a
{@link
+ * ByteBlockPool#reset()} call will advance the pool to its first buffer
immediately.
*/
public void nextBuffer() {
if (1 + bufferUpto == buffers.length) {
+ // The buffer array is full - expand it
byte[][] newBuffers =
new byte[ArrayUtil.oversize(buffers.length + 1,
NUM_BYTES_OBJECT_REF)][];
System.arraycopy(buffers, 0, newBuffers, 0, buffers.length);
buffers = newBuffers;
}
+ // Allocate new buffer and advance the pool to it
buffer = buffers[1 + bufferUpto] = allocator.getByteBlock();
bufferUpto++;
-
byteUpto = 0;
byteOffset = Math.addExact(byteOffset, BYTE_BLOCK_SIZE);
}
/**
- * Fill the provided {@link BytesRef} with the bytes at the specified
offset/length slice. This
- * will avoid copying the bytes, if the slice fits into a single block;
otherwise, it uses the
- * provided {@link BytesRefBuilder} to copy bytes over.
+ * Fill the provided {@link BytesRef} with the bytes at the specified offset
and length. This will
+ * avoid copying the bytes if the slice fits into a single block; otherwise,
it uses the provided
+ * {@link BytesRefBuilder} to copy bytes over.
*/
- void setBytesRef(BytesRefBuilder builder, BytesRef result, long offset, int
length) {
+ void setBytesRef(BytesRefBuilder builder, BytesRef result, int offset, int
length) {
result.length = length;
- int bufferIndex = (int) (offset >> BYTE_BLOCK_SHIFT);
Review Comment:
Good idea!
##########
lucene/core/src/java/org/apache/lucene/util/ByteBlockPool.java:
##########
@@ -259,14 +280,15 @@ public void readBytes(final long offset, final byte[]
bytes, int bytesOffset, in
@Override
public long ramBytesUsed() {
long size = BASE_RAM_BYTES;
- size += RamUsageEstimator.sizeOfObject(buffer);
size += RamUsageEstimator.shallowSizeOf(buffers);
for (byte[] buf : buffers) {
- if (buf == buffer) {
Review Comment:
I haven't figured out why `buffer` was a special case. Even if it were not a
full block, would that make a difference if we're calling
`RamUsageEstimator#sizeOfObject` on it in both cases?
--
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: issues-unsubscr...@lucene.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
