Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix errors in Mapped pool and javadoc #8264

Merged
merged 2 commits into from
Jul 7, 2022
Merged

Fix errors in Mapped pool and javadoc #8264

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d92fbfa
Fix errors in javadoc
gregw Jul 4, 2022
22aebc4
Further RBBP improvements
gregw Jul 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 2 additions & 3 deletions jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferPool.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,11 @@ public interface ByteBufferPool
{
/**
* <p>Requests a {@link ByteBuffer} of the given size.</p>
* <p>The returned buffer may have a bigger capacity than the size being
* requested but it will have the limit set to the given size.</p>
* <p>The returned buffer may have a bigger capacity than the size being requested.</p>
*
* @param size the size of the buffer
* @param direct whether the buffer must be direct or not
* @return the requested buffer
* @return a buffer with at least the requested capacity, with position and limit set to 0.
* @see #release(ByteBuffer)
*/
ByteBuffer acquire(int size, boolean direct);
Expand Down
32 changes: 32 additions & 0 deletions jetty-io/src/main/java/org/eclipse/jetty/io/MappedByteBufferPool.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,12 @@ private MappedByteBufferPool(int factor, int maxBucketSize, Function<Integer, Bu
_newBucket = newBucket;
}

@Override
protected RetainableByteBufferPool newRetainableByteBufferPool(int factor, int maxCapacity, int maxBucketSize, long retainedHeapMemory, long retainedDirectMemory)
{
return new Retained(factor, maxCapacity, maxBucketSize, retainedHeapMemory, retainedDirectMemory);
}

private Bucket newBucket(int key, boolean direct)
{
return (_newBucket != null) ? _newBucket.apply(key) : new Bucket(capacityFor(key), getMaxBucketSize(), updateMemory(direct));
Expand Down Expand Up @@ -302,4 +308,30 @@ public String toString()
getMaxBucketSize(),
getCapacityFactor());
}

protected class Retained extends ArrayRetainableByteBufferPool
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems strange to me that you would create a MappedByteBufferPool and ask it for its RetainableBufferPool and it gives you an array based implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, but not sure we want to create a MappedRetainableByteBuffer pool. Prior to the combination of pool, this is exactly what we would have got if the user selected a MBBP, as we would have created an ARBBP. So just a bit strange that they are together.

I'm not actually sure if we really want to keep MBBP. @sbordet @lorban why do we have it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAICT it's only used in the client, but I can't see why MBBP was created in the first place and I lack history. So unless @sbordet has a good reason to keep it, that pool impl can go IMHO.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, but for this PR, I'd like to fix javadoc and MBBP to unblock other work.
We can consider deprecating MBBP later.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me, and the change looks reasonable enough: currently MBBP is used together with a ARBBP so this change just solidifies a behavior that already exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lorban can I get a green tick then please?

{
public Retained(int factor, int maxCapacity, int maxBucketSize, long retainedHeapMemory, long retainedDirectMemory)
{
super(0, factor, maxCapacity, maxBucketSize, retainedHeapMemory, retainedDirectMemory);
}

@Override
protected ByteBuffer allocate(int capacity)
{
return MappedByteBufferPool.this.acquire(capacity, false);
}

@Override
protected ByteBuffer allocateDirect(int capacity)
{
return MappedByteBufferPool.this.acquire(capacity, true);
}

@Override
protected void removed(RetainableByteBuffer retainedBuffer)
{
MappedByteBufferPool.this.release(retainedBuffer.getBuffer());
}
}
}
57 changes: 33 additions & 24 deletions jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBufferPool.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,40 +26,49 @@ public interface RetainableByteBufferPool
* Acquires a memory buffer from the pool.
* @param size The size of the buffer. The returned buffer will have at least this capacity.
* @param direct true if a direct memory buffer is needed, false otherwise.
* @return a memory buffer.
* @return a memory buffer with position and size set to 0.
*/
RetainableByteBuffer acquire(int size, boolean direct);

void clear();

static RetainableByteBufferPool from(ByteBufferPool byteBufferPool)
{
return new RetainableByteBufferPool()
return new NotRetainedByteBufferPool(byteBufferPool);
}

class NotRetainedByteBufferPool implements RetainableByteBufferPool
{
private final ByteBufferPool _byteBufferPool;

public NotRetainedByteBufferPool(ByteBufferPool byteBufferPool)
{
@Override
public RetainableByteBuffer acquire(int size, boolean direct)
{
ByteBuffer byteBuffer = byteBufferPool.acquire(size, direct);
RetainableByteBuffer retainableByteBuffer = new RetainableByteBuffer(byteBuffer, this::release);
retainableByteBuffer.acquire();
return retainableByteBuffer;
}
_byteBufferPool = byteBufferPool;
}

private void release(RetainableByteBuffer retainedBuffer)
{
byteBufferPool.release(retainedBuffer.getBuffer());
}
@Override
public RetainableByteBuffer acquire(int size, boolean direct)
{
ByteBuffer byteBuffer = _byteBufferPool.acquire(size, direct);
RetainableByteBuffer retainableByteBuffer = new RetainableByteBuffer(byteBuffer, this::release);
retainableByteBuffer.acquire();
return retainableByteBuffer;
}

@Override
public void clear()
{
}
private void release(RetainableByteBuffer retainedBuffer)
{
_byteBufferPool.release(retainedBuffer.getBuffer());
}

@Override
public void clear()
{
}

@Override
public String toString()
{
return String.format("NonRetainableByteBufferPool@%x{%s}", hashCode(), byteBufferPool.toString());
}
};
@Override
public String toString()
{
return String.format("NonRetainableByteBufferPool@%x{%s}", hashCode(), _byteBufferPool.toString());
}
}
}
1 change: 1 addition & 0 deletions jetty-server/src/main/java/org/eclipse/jetty/server/AbstractConnector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,7 @@ public AbstractConnector(
}
}
_byteBufferPool = pool;
addBean(pool.asRetainableByteBufferPool());

addEventListener(new Container.Listener()
{
Expand Down