BufferedData.length() is incorrect if wrapped offset isn't zero

[anthony-swirldslabs]

final ByteBuffer bb = ByteBuffer.wrap(new byte[]{0, 1, 2, 3, 4, 5, 6, 7}, 1, 3);
        System.err.println("bb.lim = " + bb.limit());
        final ByteArrayBufferedData babd = new ByteArrayBufferedData(bb);
        System.err.println("babd.len = " + babd.length());

prints

bb.lim = 4
babd.len = 4

As far as the ByteBuffer is concerned, the behavior is correct per the ByteBuffer javadoc.

However, while the current ByteArrayBufferedData(ByteBuffer) ctor or a similar wrap(, offset, length) method also behave per their current javadoc, the result returned by the length() method is incorrect. We wrapped a ByteBuffer of length 3 (see the last argument at the first line of code), so the length of the BufferedData object must also be 3, which currently it isn't because:

/** {@inheritDoc} */
    @Override
    public long length() {
        return buffer.limit();
    }

and this code can only return the correct result if the offset of the buffer is zero, and also, only if the client code never modified the limit via the BufferedData APIs.

We even have unit tests that verify these odd results.

We need to revisit offsets/limits/lengths handling, fix bugs, update the unit tests, and then verify if the CN code still works and doesn't rely on the current incorrect behavior.

[anthony-swirldslabs]

It's not impossible to "fix" this bug and make the BufferedData respect the Buffer state (aka its position/limit) at the moment when the BufferedData object gets created. This would make it similar to what the MemorySegment does in JDK and hence the MemoryData just has to do in PBJ. This even improves the performance of a couple of methods since we can return a local member value instead of calling a Buffer method, such as the capacity().

However, in many other methods this fix results in adding necessary + start or - start computations, or additional length checks. While seemingly "minor", if those computations occur at a high rate, this could lead to a performance degradation.

Also, while in some methods the current implementation claims that "we can never assume ownership of the underlying buffer", in other methods it goes and uses the buffer's current position and limit freely, including modifying them. From this perspective, it's clear that the BufferedData does in fact own the underlying buffer fully, including its state w.r.t. its position and limit.

In other words, the current BufferedData is a wrapper around an existing Buffer object with all its current, however changing, state intact, rather than a view into a slice of the buffer at the moment when the BufferedData object is instantiated. However, there's a few javadocs that make it look as if it's just a view.

Apart from just a couple of quirks in the BufferedData unit tests that now have to differentiate between the actual BufferedData and the new MemoryData objects, there's not any other reports from users where they encounter problems with the current BufferedData behavior when it comes to its position, limit, offsets, etc. Given that PBJ aims for the performance, and the current approach is more performant than a "fixed" one, and there's no actual bugs in client code caused by the current behavior, it seems reasonable to document the existing behavior in the javadoc, rather than change it and incur some performance degradation related to the necessary extra computations.