Can BufferRecycler support a fixed buffer size which can be customized, instead of dynamic one by calling _allocMore()

[qiuyinglanshan]

As _allocMore() method is called, _byteBuffers will reference to a new larger byte array, and the garbage collector has to collect the original memory. In my application, it may cause frequently fullgc.

The screenshot shows a dump with many unreachable byte array (in old heap) which caused by BufferRecycler.

When I switched USE_THREAD_LOCAL_FOR_BUFFER_RECYCLING to false, fullgc times reduced while younggc times increased, so I ask you if we can use bufferRecycler and with a fixed size buffer ?

[cowtowncoder]

Unfortunately due to the way BufferRecycler works it is difficult to add configurability (mostly it being handled with ThreadLocal and there's nothing bound to JsonFactory unlike most other things are).
But I do think that what you asking (ability to limit maximum recyclable buffer size, possibly by limiting largest chunk to allocate) makes sense.

I'll leave this open and see if something could be figured out.

[qiuyinglanshan]

@cowtowncoder
Thanks for your reply.

I found that when releasing space to BufferRecycler, comparing the size of the current space with the original space(recorded in an array at each allocation) can solve the problem. Could you help confirm whether the following codes are correct?

• init:

    private final byte[][] _byteBuffersRecords;
    private final char[][] _charBuffersRecords;

    protected BufferRecycler(int bbCount, int cbCount) {
        _byteBuffers = new AtomicReferenceArray<byte[]>(bbCount);
        _charBuffers = new AtomicReferenceArray<char[]>(cbCount);
        _byteBuffersRecords = new byte[bbCount][];
        _charBuffersRecords = new char[cbCount][];
    }

• alloc:

    public byte[] allocByteBuffer(int ix, int minSize) {
        final int DEF_SIZE = byteBufferLength(ix);
        if (minSize < DEF_SIZE) {
            minSize = DEF_SIZE;
        }
        byte[] buffer = _byteBuffers.getAndSet(ix, null);
        if (buffer == null || buffer.length < minSize) {
            buffer = balloc(minSize);
        } else {
            // fill in records
            _byteBuffersRecords[ix] = buffer;
        }
        return buffer;
    }

• release:

    public void releaseByteBuffer(int ix, byte[] buffer) {
        if (_byteBuffersRecords[ix] != null) {
            if (buffer == null || _byteBuffersRecords[ix].length >= buffer.length) {
                // get from records
                buffer = _byteBuffersRecords[ix];
            }
            _byteBuffersRecords[ix] = null;
        }
        _byteBuffers.set(ix, buffer);
    }

This ensures that if the newly created buffer length is less than or equal to the original, it will not be updated. I think this can reduce the number of buffer objects promoted to the old generation space to some extent.

[cowtowncoder]

@qiuyinglanshan I think I understand the idea and it makes sense (try to avoid replacing buffer with something that's not bigger than what we already had). But I do not like storing an additional reference in separate array.
How about instead check value _byteBuffers (or _charBuffers) entry has, if any, and compare size it has? And then only set if there was no previous value OR released buffer is bigger than previous one?

I'll see if I can provide a simple alternative that does this.

[cowtowncoder]

Oh actually, no, never mind. I forgot that the buffer reference is cleared when returned with _byteBuffers.getAndSet(ix, null);. So there's no way to compare size.

So with this I am not sure why change you suggest would make any difference. I am not sure I understand the logic after all.

[qiuyinglanshan]

Oh actually, no, never mind. I forgot that the buffer reference is cleared when returned with _byteBuffers.getAndSet(ix, null);. So there's no way to compare size.

Yes, this is why I created a reference point to the original byte array instead of comparing sizes directly.

So with this I am not sure why change you suggest would make any difference. I am not sure I understand the logic after all.

I'll try to show the difference between the original logic and the new logic.

First of all, I want to explain that because _byteBuffers is held by ThreadLocal for a long time (eg. in thread pool case), and the array object it references will be promoted to the old age.

When releaseByteBuffer() is called, _byteBuffers may refer to a new array object if _allocMore() is executed. Although with the occurrence of younggc for many times(eg. max 15 age), this new object has not been recycled since ThreadLocal holding. As a result, the new array object will continue to promoted the old generation space.

My new logic is to hope that the new array object can be recycled by younggc if its length less than or equal to the original buffer arrry(In many cases, both of the length are 128k). So it reduce the number of buffer objects promoted to the old generation space, and fullgc occurs less frequently.

[qiuyinglanshan]

In addition, as you mentioned in #835 , I think it would be better to add an initial buffer size that can be configured instead of "{ 8000, 8000, 2000, 2000 }".
For example, in my case, I would set the initial buffer size to 128k without using _allocMore to expand it, thus reducing unnecessary performance overhead and long alive object garbage.

[cowtowncoder]

+1 for configurability, created #835 for being able to configure BufferRecycler behavior or add a custom implementation.

As to change proposed, part I do not understand is why change would likely change things: _allocMore() should usually only be called to allocate bigger buffers. I guess the one case where it might matter would be when the max block size is reached and new block is same size (max size). I can see how that could cause churn.

On change proposed, I think there is a real correctness problem for async use cases where same thread may be used to serve multiple requests. If so, when a buffer is returned we can NOT be certain that all buffers returned are freed (or more specifically, most recently returned buffer has been freed). So I think making change could cause data corruption where sometimes (rarely but sometimes) buffer could end up being used by 2 parsers/generators, from different sync handlers. I say this because originally reference to buffer was not actually cleared and we relied on assumption that calls are always one-thread-per-request blocking code.
So to solve the problem of returning different-but-no-bigger buffer it'd have to be solved from side of entity that gets and releases buffer. Not sure how feasible that is.

[qiuyinglanshan]

I'm sorry I can not understand it, and I'd appreciate it if you could point it out again.

So I think making change could cause data corruption where sometimes (rarely but sometimes) buffer could end up being used by 2 parsers/generators, from different sync handlers.

According to the following code, when a buffer is borrowed, it will NOT be borrowed again by other requests until it is returned by the previous request. Therefore, the buffer(and more specifically, the index it is in) should be exclusive, so I cannot understand why it will NOT be freed by other requests when it is returned.

    byte[] buffer = _byteBuffers.getAndSet(ix, null);
    if (buffer == null || buffer.length < minSize) {
        buffer = balloc(minSize);
    } 

when a buffer is returned we can NOT be certain that all buffers returned are freed

The newly added reference is actually an array of 4 references, with each element pointing to a unique buffer when it is borrowed. In the new logic, only the current index(variable ix) is processed, and no other buffers are processed.

    if (_byteBuffersRecords[ix] != null) {
        if (buffer == null || _byteBuffersRecords[ix].length >= buffer.length) {
            // get from records
            buffer = _byteBuffersRecords[ix];
        }
        _byteBuffersRecords[ix] = null;
    }

Looking forward to your guidance~

[cowtowncoder]

@qiuyinglanshan You are assuming that there will not be overlapping creation and usage of parsers and/or generators: this is not true for asynchronous frameworks -- or actually even in blocking ones, come to think of that.
There is nothing to enforce the original assumed allocate/release cycle. It would be possible to create a test scenario for this, constructing one parser, calling nextToken() on it, creating another one from same thread, calling nextToken(), then closing first one. The most recent allocate would not match with the release.

I don't think I want to proceed with approach suggested above. If the allocation churn is to be solved, it will require caller (parser, generator) to try to return what it seems is most optimal (biggest, or if multiple buffers with same size allocated, the one that is recycled).

[cowtowncoder]

A lot of work went into 2.16 to allow pluggable recycler pools (see JsonRecyclerPools, #1064.

Closing this issue as it is probably outdated; if changes still desired, should file a new issue based on current state of things (wrt 2.17 branch).