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.

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

move pool reference from BufferRecycler to IOContext #1107

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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: 7 additions & 17 deletions src/main/java/com/fasterxml/jackson/core/JsonFactory.java
Original file line number Diff line number Diff line change
Expand Up @@ -2140,26 +2140,13 @@ protected JsonGenerator _decorate(JsonGenerator g) {
/**********************************************************
*/

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

/**
* Accessor for getting access to {@link BufferRecyclerPool} for getting
* {@link BufferRecycler} instance to use.
*
* @since 2.16
*/
public BufferRecyclerPool _getBufferRecyclerPool() {
private BufferRecyclerPool _getBufferRecyclerPool() {
// 23-Apr-2015, tatu: Let's allow disabling of buffer recycling
// scheme, for cases where it is considered harmful (possibly
// on Android, for example)
Expand All @@ -2183,8 +2170,9 @@ protected IOContext _createContext(ContentReference contentRef, boolean resource
if (contentRef == null) {
contentRef = ContentReference.unknown();
}
BufferRecyclerPool pool = _getBufferRecyclerPool();
return new IOContext(_streamReadConstraints, _streamWriteConstraints, _errorReportConfiguration,
_getBufferRecycler(), contentRef, resourceManaged);
pool.acquireBufferRecycler(), pool, contentRef, resourceManaged);
}

/**
Expand All @@ -2199,8 +2187,9 @@ protected IOContext _createContext(ContentReference contentRef, boolean resource
*/
@Deprecated // @since 2.13
protected IOContext _createContext(Object rawContentRef, boolean resourceManaged) {
BufferRecyclerPool pool = _getBufferRecyclerPool();
return new IOContext(_streamReadConstraints, _streamWriteConstraints, _errorReportConfiguration,
_getBufferRecycler(),
Copy link
Member

Choose a reason for hiding this comment

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

Alas, I don't like this, due to backwards-(in)compatibility aspect -- this will immediately break usage by those JsonFactory sub-classes that override this method (I think CSV one does, probably others).
Or rather, require synchronized changes (technically allowed as per my "internal vs external" compatibility goals.. as long as all 2.16 components work together).

pool.acquireBufferRecycler(), pool,
_createContentReference(rawContentRef),
resourceManaged);
}
Expand All @@ -2218,8 +2207,9 @@ protected IOContext _createContext(Object rawContentRef, boolean resourceManaged
protected IOContext _createNonBlockingContext(Object srcRef) {
// [jackson-core#479]: allow recycling for non-blocking parser again
// now that access is thread-safe
BufferRecyclerPool pool = _getBufferRecyclerPool();
return new IOContext(_streamReadConstraints, _streamWriteConstraints, _errorReportConfiguration,
_getBufferRecycler(),
pool.acquireBufferRecycler(), pool,
_createContentReference(srcRef),
false);
}
Expand Down
15 changes: 11 additions & 4 deletions src/main/java/com/fasterxml/jackson/core/io/IOContext.java
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import com.fasterxml.jackson.core.StreamReadConstraints;
import com.fasterxml.jackson.core.StreamWriteConstraints;
import com.fasterxml.jackson.core.util.BufferRecycler;
import com.fasterxml.jackson.core.util.BufferRecyclerPool;
import com.fasterxml.jackson.core.util.ReadConstrainedTextBuffer;
import com.fasterxml.jackson.core.util.TextBuffer;

Expand Down Expand Up @@ -64,6 +65,8 @@ public class IOContext implements AutoCloseable
*/
protected final BufferRecycler _bufferRecycler;

private final BufferRecyclerPool _bufferRecyclerPool;

/**
* @since 2.15
*/
Expand Down Expand Up @@ -133,19 +136,21 @@ public class IOContext implements AutoCloseable
* @param src constraints for streaming reads
* @param swc constraints for streaming writes
* @param br BufferRecycler to use, if any ({@code null} if none)
* @param brPool BufferRecyclerPool the pool from where the BufferRecycler has been acquired, 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 erc Error report configuration to use
*
* @since 2.16
*/
public IOContext(StreamReadConstraints src, StreamWriteConstraints swc, ErrorReportConfiguration erc,
BufferRecycler br, ContentReference contentRef, boolean managedResource)
BufferRecycler br, BufferRecyclerPool brPool, ContentReference contentRef, boolean managedResource)
{
_streamReadConstraints = src;
_streamWriteConstraints = swc;
_errorReportConfiguration = erc;
_bufferRecycler = br;
_bufferRecyclerPool = brPool;
_contentReference = contentRef;
_sourceRef = contentRef.getRawContent();
_managedResource = managedResource;
Expand All @@ -168,7 +173,7 @@ public IOContext(StreamReadConstraints src, BufferRecycler br,
ContentReference contentRef, boolean managedResource)
{
this(src, StreamWriteConstraints.defaults(), ErrorReportConfiguration.defaults(),
br, contentRef, managedResource);
br, null, contentRef, managedResource);
}

/**
Expand All @@ -186,7 +191,7 @@ public IOContext(StreamReadConstraints src, BufferRecycler br,
public IOContext(BufferRecycler br, ContentReference contentRef, boolean managedResource)
{
this(StreamReadConstraints.defaults(), StreamWriteConstraints.defaults(), ErrorReportConfiguration.defaults(),
br, contentRef, managedResource);
br, null, contentRef, managedResource);
}

@Deprecated // since 2.13
Expand Down Expand Up @@ -464,7 +469,9 @@ private IllegalArgumentException wrongBuf() {
@Override
public void close() {
if (!_closed) {
_bufferRecycler.release();
if (_bufferRecyclerPool != null) {
_bufferRecycler.release(_bufferRecyclerPool);
}
_closed = true;
}
}
Expand Down
31 changes: 3 additions & 28 deletions src/main/java/com/fasterxml/jackson/core/util/BufferRecycler.java
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,6 @@ public class BufferRecycler
// Note: changed from simple array in 2.10:
protected final AtomicReferenceArray<char[]> _charBuffers;

private BufferRecyclerPool _pool;

/*
/**********************************************************
/* Construction
Expand Down Expand Up @@ -197,34 +195,11 @@ protected int charBufferLength(int ix) {
protected char[] calloc(int size) { return new char[size]; }

/**
* Method called by owner of this recycler instance, to provide reference to
* {@link BufferRecyclerPool} into which instance is to be released (if any)
* Method called when owner of this recycler no longer wishes use it.
*
* @since 2.16
*/
BufferRecycler withPool(BufferRecyclerPool pool) {
if (this._pool != null) {
throw new IllegalStateException("BufferRecycler already linked to pool: "+pool);
}
// assign to pool to which this BufferRecycler belongs in order to release it
// to the same pool when the work will be completed
_pool = Objects.requireNonNull(pool);
return this;
}

/**
* Method called when owner of this recycler no longer wishes use it; this should
* return it to pool passed via {@code withPool()} (if any).
*
* @since 2.16
*/
public void release() {
if (_pool != null) {
BufferRecyclerPool tmpPool = _pool;
// nullify the reference to the pool in order to avoid the risk of releasing
// the same BufferRecycler more than once, thus compromising the pool integrity
_pool = null;
tmpPool.releaseBufferRecycler(this);
}
public void release(BufferRecyclerPool pool) {
pool.releaseBufferRecycler(this);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,7 @@ public BufferRecycler acquireBufferRecycler() {
if (bufferRecycler == null) {
bufferRecycler = new BufferRecycler();
}
return bufferRecycler.withPool(this);
return bufferRecycler;
}

@Override
Expand Down Expand Up @@ -333,7 +333,7 @@ protected Object readResolve() {

@Override
public BufferRecycler acquireBufferRecycler() {
return _getRecycler().withPool(this);
return _getRecycler();
}

private BufferRecycler _getRecycler() {
Expand Down Expand Up @@ -448,7 +448,7 @@ public BufferRecycler acquireBufferRecycler() {
if (bufferRecycler == null) {
bufferRecycler = new BufferRecycler();
}
return bufferRecycler.withPool(this);
return bufferRecycler;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@ public void testBounded() throws Exception {
checkBufferRecyclerPoolImpl(BufferRecyclerPool.BoundedPool.nonShared(1), true);
}

public void testPluggingPool() throws Exception {
checkBufferRecyclerPoolImpl(new TestPool(), true);
}

private void checkBufferRecyclerPoolImpl(BufferRecyclerPool pool, boolean checkPooledResource) throws Exception {
JsonFactory jsonFactory = JsonFactory.builder()
.bufferRecyclerPool(pool)
Expand All @@ -45,7 +49,7 @@ private void checkBufferRecyclerPoolImpl(BufferRecyclerPool pool, boolean checkP
try {
assertSame(usedBufferRecycler, pooledBufferRecycler);
} finally {
pooledBufferRecycler.release();
pooledBufferRecycler.release(pool);
}
}
}
Expand Down Expand Up @@ -75,4 +79,23 @@ private static class NopOutputStream extends OutputStream {
@Override
public void write(byte[] b, int offset, int len) throws IOException { size += len; }
}

class TestPool implements BufferRecyclerPool {
private BufferRecycler bufferRecycler;

@Override
public BufferRecycler acquireBufferRecycler() {
if (bufferRecycler != null) {
BufferRecycler tmp = bufferRecycler;
this.bufferRecycler = null;
return tmp;
}
return new BufferRecycler();
}

@Override
public void releaseBufferRecycler(BufferRecycler recycler) {
this.bufferRecycler = recycler;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ public void testAllocations() throws Exception
IOContext ctxt = new IOContext(StreamReadConstraints.defaults(),
StreamWriteConstraints.defaults(),
ErrorReportConfiguration.defaults(),
new BufferRecycler(),
new BufferRecycler(), null,
ContentReference.rawReference("N/A"), true);

/* I/O Read buffer */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,6 @@ private static IOContext testIOContext(StreamReadConstraints src,
StreamWriteConstraints swc,
ErrorReportConfiguration erc) {
return new IOContext(src, swc, erc,
new BufferRecycler(), ContentReference.unknown(), false);
new BufferRecycler(), null, ContentReference.unknown(), false);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ private static IOContext makeConstrainedContext(int maxStringLen) {
constraints,
StreamWriteConstraints.defaults(),
ErrorReportConfiguration.defaults(),
new BufferRecycler(),
new BufferRecycler(), null,
ContentReference.rawReference("N/A"), true);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ private IOContext _ioContext(StreamWriteConstraints swc) {
return new IOContext(StreamReadConstraints.defaults(),
swc,
ErrorReportConfiguration.defaults(),
new BufferRecycler(),
new BufferRecycler(), null,
ContentReference.unknown(), true);
}
}