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

introducing BufferRecyclerPool #1061

Closed
wants to merge 1 commit into from
Closed

introducing BufferRecyclerPool #1061

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 2 additions & 22 deletions src/main/java/tools/jackson/core/TokenStreamFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
import tools.jackson.core.sym.PropertyNameMatcher;
import tools.jackson.core.sym.SimpleNameMatcher;
import tools.jackson.core.util.BufferRecycler;
import tools.jackson.core.util.BufferRecyclers;
import tools.jackson.core.util.JacksonFeature;
import tools.jackson.core.util.Named;
import tools.jackson.core.util.Snapshottable;
Expand Down Expand Up @@ -1182,25 +1181,6 @@ public JsonGenerator createGenerator(Writer w) throws JacksonException {
/**********************************************************************
*/

/**
* Method used by factory to create buffer recycler instances
* for parsers and generators.
*<p>
* Note: only public to give access for {@code ObjectMapper}
*
* @return BufferRecycler instance to use
*/
public BufferRecycler _getBufferRecycler()
Copy link
Member

Choose a reason for hiding this comment

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

It's unlikely that we will accept having any public APIs removed - you will ideally need to keep them and possibly deprecate them.

Otherwise, this PR will need to be retargeted and master and aimed at the 3.0 release which has no release date set for it.

Copy link
Member

Choose a reason for hiding this comment

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

I see this is targeted at master - but be aware, it could be years before 3.0 is released.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I indeed sent this PR against master, as I expect this to be ready and merged for the 3.0 release. I don't think that we could keep alive both this method and the solution based on the new object pool at the same time, because using one will break the assumptions made by the other. That said my understanding was that the methods and fields starting with an underscore were intended only for internal use and not considered part of the public API, am I missing something?

Copy link
Contributor Author

@mariofusco mariofusco Jul 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I see this is targeted at master - but be aware, it could be years before 3.0 is released.

Ok, if so I guess this won't be acceptable for our usage purposes in Quarkus. Should I retarget this against 2.16 branch? Also can you clarify the naming convention around method names starting with an underscore? Is it safe to assume that they are not part of the public API?

Copy link
Member

Choose a reason for hiding this comment

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

public means public. @cowtowncoder may consider allowing an exception - he's the BDFL.

Since it's following the underscore naming convention, you might say it is public but not guaranteed not to be removed at short notice (not up to me to make this call though).

{
// 23-Apr-2015, tatu: Let's allow disabling of buffer recycling
// scheme, for cases where it is considered harmful (possibly
// on Android, for example)
if (Feature.USE_THREAD_LOCAL_FOR_BUFFER_RECYCLING.enabledIn(_factoryFeatures)) {
return BufferRecyclers.getBufferRecycler();
}
return new BufferRecycler();
}

/**
* Overridable factory method that actually instantiates desired
* context object.
Expand All @@ -1212,7 +1192,7 @@ public BufferRecycler _getBufferRecycler()
*/
protected IOContext _createContext(ContentReference contentRef, boolean resourceManaged) {
return new IOContext(_streamReadConstraints, _streamWriteConstraints,
_getBufferRecycler(), contentRef,
contentRef,
resourceManaged, null);
}

Expand All @@ -1229,7 +1209,7 @@ protected IOContext _createContext(ContentReference contentRef, boolean resource
protected IOContext _createContext(ContentReference contentRef, boolean resourceManaged,
JsonEncoding enc) {
return new IOContext(_streamReadConstraints, _streamWriteConstraints,
_getBufferRecycler(), contentRef,
contentRef,
resourceManaged, enc);
}

Expand Down
6 changes: 5 additions & 1 deletion src/main/java/tools/jackson/core/base/GeneratorBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,11 @@ public JsonGenerator writeTree(TreeNode rootNode) throws JacksonException {
*/

// @Override public abstract void flush();
@Override public void close() { _closed = true; }
@Override public void close() {
_closed = true;
_ioContext.close();
}

@Override public boolean isClosed() { return _closed; }

/*
Expand Down
1 change: 1 addition & 0 deletions src/main/java/tools/jackson/core/base/ParserBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,7 @@ public Object currentValue() {
// as per [JACKSON-324], do in finally block
// Also, internal buffer(s) can now be released as well
_releaseBuffers();
_ioContext.close();
}
}
}
Expand Down
18 changes: 14 additions & 4 deletions src/main/java/tools/jackson/core/io/IOContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
package tools.jackson.core.io;

import java.util.Objects;
import java.util.concurrent.atomic.AtomicBoolean;

import tools.jackson.core.JsonEncoding;
import tools.jackson.core.StreamReadConstraints;
import tools.jackson.core.StreamWriteConstraints;
import tools.jackson.core.util.BufferRecycler;
import tools.jackson.core.util.BufferRecyclerPool;
import tools.jackson.core.util.TextBuffer;
import tools.jackson.core.util.ReadConstrainedTextBuffer;

Expand All @@ -15,7 +17,7 @@
* readers and writers are combined under this object. One instance
* is created for each reader and writer.
*/
public class IOContext
public class IOContext implements AutoCloseable
{
/*
/**********************************************************************
Expand Down Expand Up @@ -99,6 +101,8 @@ public class IOContext
*/
protected char[] _nameCopyBuffer;

private volatile AtomicBoolean closed = new AtomicBoolean(false);
Copy link

Choose a reason for hiding this comment

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

as per later, maybe not needed any atomic type, if really need it - prefer AtomicIntegerFieldUpdater (no bool sadly :"( unless using VarHandle, which is probably not available for the base supported JDK version) or make it final and not volatile (itself) too.

The suggestion for the field updater is due to the number of IOContext instances alive, to reduce the footprint.

Copy link
Member

Choose a reason for hiding this comment

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

Not quite sure how one could reduce footprint as IOContexts are independent and independently handled?


/*
/**********************************************************************
/* Life-cycle
Expand All @@ -110,19 +114,18 @@ public class IOContext
*
* @param streamReadConstraints constraints for streaming reads
* @param streamWriteConstraints constraints for streaming writes
* @param br BufferRecycler to use, if any ({@code null} if none)
* @param contentRef Input source reference for location reporting
* @param managedResource Whether input source is managed (owned) by Jackson library
* @param enc Encoding in use
*/
public IOContext(StreamReadConstraints streamReadConstraints,
StreamWriteConstraints streamWriteConstraints,
BufferRecycler br, ContentReference contentRef, boolean managedResource,
ContentReference contentRef, boolean managedResource,
JsonEncoding enc)
{
_streamReadConstraints = Objects.requireNonNull(streamReadConstraints);
_streamWriteConstraints = Objects.requireNonNull(streamWriteConstraints);
_bufferRecycler = br;
_bufferRecycler = BufferRecyclerPool.borrowBufferRecycler();
_contentReference = contentRef;
_managedResource = managedResource;
_encoding = enc;
Expand Down Expand Up @@ -362,4 +365,11 @@ private IllegalArgumentException wrongBuf() {
// sanity check failed; trying to return different, smaller buffer.
return new IllegalArgumentException("Trying to release buffer smaller than original");
}

@Override
public void close() {
if (closed.compareAndSet(false, true)) {
Copy link

Choose a reason for hiding this comment

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

This is supposed to be concurrent? If not, and just "ordered" you can use a plain bool

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now I implemented it in a concurrent way, but I agree that this constraint can be relaxed if not necessary.

BufferRecyclerPool.offerBufferRecycler(_bufferRecycler);
Copy link
Member

Choose a reason for hiding this comment

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

If final definition was removed, we could perhaps use null-ness of _bufferRecycler itself (i.e. only offer if not null)

}
}
}
2 changes: 1 addition & 1 deletion src/main/java/tools/jackson/core/json/JsonFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,7 +351,7 @@ public JsonParser createNonBlockingByteBufferParser(ObjectReadContext readCtxt)
}

protected IOContext _createNonBlockingContext(Object srcRef) {
return new IOContext(_streamReadConstraints, _streamWriteConstraints, _getBufferRecycler(),
return new IOContext(_streamReadConstraints, _streamWriteConstraints,
ContentReference.rawReference(srcRef), false, JsonEncoding.UTF8);
}

Expand Down
13 changes: 13 additions & 0 deletions src/main/java/tools/jackson/core/util/BufferRecyclerPool.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package tools.jackson.core.util;

public class BufferRecyclerPool {

private static final ObjectPool<BufferRecycler> pool = ObjectPool.newLockFreePool(BufferRecycler::new);
Copy link
Member

Choose a reason for hiding this comment

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

Ok; since this is now global pool (across all factories), there definitely should be maximum size. But then again, having such setting would lead to need for configuration.
And static singletons are generally a bad idea and not suitable for use by (low-level) libraries.

In fact, now that I think about this, I think pooling (or default implementation at any rate) really needs to be segmented by factory instance -- no buffers should be shared (by default impl) across TokenStreamFactory instances.


public static BufferRecycler borrowBufferRecycler() {
return pool.borrow();
}
public static void offerBufferRecycler(BufferRecycler bufferRecycler) {
pool.offer(bufferRecycler);
}
}
65 changes: 65 additions & 0 deletions src/main/java/tools/jackson/core/util/ObjectPool.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
package tools.jackson.core.util;

import java.util.Queue;
import java.util.concurrent.LinkedTransferQueue;
import java.util.function.Consumer;
import java.util.function.Supplier;

public interface ObjectPool<T> extends AutoCloseable {

T borrow();
void offer(T t);

default void withPooledObject(Consumer<T> objectConsumer) {
T t = borrow();
try {
objectConsumer.accept(t);
} finally {
offer(t);
}
}

static <T> ObjectPool<T> newLockFreePool(Supplier<T> factory) {
return new LockFreeObjectPool<>(factory);
}

static <T> ObjectPool<T> newLockFreePool(Supplier<T> factory, Consumer<T> destroyer) {
return new LockFreeObjectPool<>(factory, destroyer);
}

class LockFreeObjectPool<T> implements ObjectPool<T> {
Copy link
Member

Choose a reason for hiding this comment

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

I think that any pool to use should have maximum size, as a general principle.

private final Supplier<T> factory;
private final Consumer<T> destroyer;

private final Queue<T> pool = new LinkedTransferQueue<>();

public LockFreeObjectPool(Supplier<T> factory) {
this(factory, null);
}

public LockFreeObjectPool(Supplier<T> factory, Consumer<T> destroyer) {
this.factory = factory;
this.destroyer = destroyer;
}

@Override
public T borrow() {
// System.out.println("Before borrow size: " + pool.size());
T t = pool.poll();
return t != null ? t : factory.get();
}

@Override
public void offer(T t) {
pool.offer(t);
// System.out.println("After offer size: " + pool.size());
}

@Override
public void close() throws Exception {
if (destroyer != null) {
pool.forEach(destroyer);
}
}
}
}
1 change: 0 additions & 1 deletion src/test/java/tools/jackson/core/BaseTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -622,7 +622,6 @@ protected JsonFactoryBuilder streamFactoryBuilder() {
protected IOContext ioContextForTests(Object source) {
return new IOContext(StreamReadConstraints.defaults(),
StreamWriteConstraints.defaults(),
new BufferRecycler(),
ContentReference.rawReference(source), true,
JsonEncoding.UTF8);
}
Expand Down
1 change: 0 additions & 1 deletion src/test/java/tools/jackson/core/io/TestIOContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ public void testAllocations() throws Exception
{
IOContext ctxt = new IOContext(StreamReadConstraints.defaults(),
StreamWriteConstraints.defaults(),
new BufferRecycler(),
ContentReference.rawReference("N/A"), true,
JsonEncoding.UTF8);

Expand Down
1 change: 0 additions & 1 deletion src/test/java/tools/jackson/core/io/TestMergedStream.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ public void testSimple() throws Exception
{
IOContext ctxt = new IOContext(StreamReadConstraints.defaults(),
StreamWriteConstraints.defaults(),
new BufferRecycler(),
ContentReference.UNKNOWN_CONTENT, false, JsonEncoding.UTF8);
// bit complicated; must use recyclable buffer...
byte[] first = ctxt.allocReadIOBuffer();
Expand Down
15 changes: 5 additions & 10 deletions src/test/java/tools/jackson/core/io/UTF8WriterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,12 +105,11 @@ public void testSurrogatesOk() throws Exception
@SuppressWarnings("resource")
public void testSurrogatesFail() throws Exception
{
BufferRecycler rec = new BufferRecycler();
ByteArrayOutputStream out;
UTF8Writer w;

out = new ByteArrayOutputStream();
w = new UTF8Writer( _ioContext(rec), out);
w = new UTF8Writer( _ioContext(), out);
try {
w.write(0xDE03);
fail("should not pass");
Expand All @@ -119,7 +118,7 @@ public void testSurrogatesFail() throws Exception
}

out = new ByteArrayOutputStream();
w = new UTF8Writer(_ioContext(rec), out);
w = new UTF8Writer(_ioContext(), out);
w.write(0xD83D);
try {
w.write('a');
Expand All @@ -129,7 +128,7 @@ public void testSurrogatesFail() throws Exception
}

out = new ByteArrayOutputStream();
w = new UTF8Writer(_ioContext(rec), out);
w = new UTF8Writer(_ioContext(), out);
try {
w.write("\uDE03");
fail("should not pass");
Expand All @@ -138,7 +137,7 @@ public void testSurrogatesFail() throws Exception
}

out = new ByteArrayOutputStream();
w = new UTF8Writer(_ioContext(rec), out);
w = new UTF8Writer(_ioContext(), out);
try {
w.write("\uD83Da");
fail("should not pass");
Expand All @@ -148,11 +147,7 @@ public void testSurrogatesFail() throws Exception
}

private IOContext _ioContext() {
return _ioContext(new BufferRecycler());
}

private IOContext _ioContext(BufferRecycler br) {
return new IOContext(StreamReadConstraints.defaults(), StreamWriteConstraints.defaults(), br,
return new IOContext(StreamReadConstraints.defaults(), StreamWriteConstraints.defaults(),
ContentReference.unknown(), false, JsonEncoding.UTF8);
}
}
1 change: 0 additions & 1 deletion src/test/java/tools/jackson/core/util/ReadConstrainedTextBufferTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ private static TextBuffer makeConstrainedBuffer(int maxStringLen) {
IOContext ioContext = new IOContext(
streamReadConstraints,
StreamWriteConstraints.defaults(),
new BufferRecycler(),
ContentReference.rawReference("N/A"), true, JsonEncoding.UTF8);
return ioContext.constructReadConstrainedTextBuffer();
}
Expand Down
2 changes: 0 additions & 2 deletions src/test/java/tools/jackson/core/write/UTF8GeneratorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ public void testNestingDepthWithSmallLimit() throws Exception
ByteArrayOutputStream bytes = new ByteArrayOutputStream();
IOContext ioc = new IOContext(StreamReadConstraints.defaults(),
StreamWriteConstraints.builder().maxNestingDepth(1).build(),
new BufferRecycler(),
ContentReference.rawReference(bytes), true,
JsonEncoding.UTF8);
try (JsonGenerator gen = new UTF8JsonGenerator(ObjectWriteContext.empty(), ioc, 0, 0, bytes,
Expand All @@ -72,7 +71,6 @@ public void testNestingDepthWithSmallLimitNestedObject() throws Exception
ByteArrayOutputStream bytes = new ByteArrayOutputStream();
IOContext ioc = new IOContext(StreamReadConstraints.defaults(),
StreamWriteConstraints.builder().maxNestingDepth(1).build(),
new BufferRecycler(),
ContentReference.rawReference(bytes), true,
JsonEncoding.UTF8);
try (JsonGenerator gen = new UTF8JsonGenerator(ObjectWriteContext.empty(), ioc, 0, 0, bytes,
Expand Down
2 changes: 0 additions & 2 deletions src/test/java/tools/jackson/core/write/WriterBasedJsonGeneratorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ public void testNestingDepthWithSmallLimit() throws Exception
StringWriter sw = new StringWriter();
IOContext ioc = new IOContext(StreamReadConstraints.defaults(),
StreamWriteConstraints.builder().maxNestingDepth(1).build(),
new BufferRecycler(),
ContentReference.rawReference(sw), true,
JsonEncoding.UTF8);
try (JsonGenerator gen = new WriterBasedJsonGenerator(ObjectWriteContext.empty(), ioc, 0, 0, sw,
Expand All @@ -43,7 +42,6 @@ public void testNestingDepthWithSmallLimitNestedObject() throws Exception
StringWriter sw = new StringWriter();
IOContext ioc = new IOContext(StreamReadConstraints.defaults(),
StreamWriteConstraints.builder().maxNestingDepth(1).build(),
new BufferRecycler(),
ContentReference.rawReference(sw), true,
JsonEncoding.UTF8);
try (JsonGenerator gen = new WriterBasedJsonGenerator(ObjectWriteContext.empty(), ioc, 0, 0, sw,
Expand Down