feat: Adding PbjReader

[ldintr]

New PbjReader class: varint/fixed-width/byte/boolean/string/bytes reads over a reusable ~16KB buffer, constructible from byte[]/ByteBuffer/Bytes/InputStream/ReadableSequentialData. Uses a sticky error code instead of throwing on every read; a system property lets callers trade real stack traces for shared exception instances. Adds supporting ParseException constructors and Bytes.resetPbjReader.

Overview: PbjReader/PbjWriter are a leaner, buffer-reusing alternative to the existing ReadableSequentialData/WritableSequentialData read/write path used by Codec, ProtoParserTools/ProtoWriterTools, and the pbj-compiler generators. They reuse an internal ~16KB (L1-cache-sized) buffer across many parse/write calls instead of allocating per call, avoid checked-exception-based control flow by recording a sticky internal error code instead, and split hot-path operations (e.g. zigzag vs. non-zigzag varints, direct UTF-8 decode into a reusable char[]) into dedicated fast methods.

[github-actions]

JUnit Test Report

   521 files  ±0     521 suites  ±0   28s ⏱️ -3s
 1 537 tests ±0   1 533 ✅ ±0   4 💤 ±0  0 ❌ ±0 
10 755 runs  ±0  10 727 ✅ ±0  28 💤 ±0  0 ❌ ±0 

Results for commit d192c9c. ± Comparison against base commit b657b11.

♻️ This comment has been updated with latest results.

[github-actions]

Integration Test Report

    428 files  ±0      428 suites  ±0   24m 36s ⏱️ + 8m 54s
115 048 tests ±0  115 048 ✅ ±0  0 💤 ±0  0 ❌ ±0 
115 292 runs  ±0  115 292 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit d192c9c. ± Comparison against base commit b657b11.

♻️ This comment has been updated with latest results.

[imalygin]

Please, add meaningful PR description which includes motivation for this change and add unit tests for PbjReader.

[artemananiev]

Why is there a separate construct() for byte arrays, but one construct() for RSD and InputStream? Should there be three, one for each type? Should there be one with all possible input types?

[ldintr]

I looked at it again, the ones with bytes are completely different than the stream that there'd be no overlap. The two streams are very similar but I have no problem separating them

[artemananiev]

Do I get it right that the whole purpose of all these resetWith() methods is to avoid creating new PbjReader instances, but reusing existing objects instead?

As far as I can see, resetWith() calls construct(), which throws away all the state of the current PbjReader object and assigns all fields new values. This may be slower than to create a new object (it will be short-living, so the changes are it will be in the young GC generation). The only field not reassigned seems to be ownedBuf, is it what really drives the reuses?

[ldintr]

The project I'm on is running into allocation issues. There's too many and the gc is doing a lot of work. Reusing helps them and if someone finds a situation where it's faster to use new they still can do it

[artemananiev]

This is (one of) the reason I proposed previously to get rid of error codes. If err is not zero, this means there was a problem in past. If exceptions were used instead of error codes, the stack would be immediately unwinded to the caller. No way the caller could forget to check the error code, they would be forced to catch (and handle) the exceptions.

For now, this just pollutes the code. And this is error prone.

[ldintr]

I'm not sure if that's ever hit in practice. I don't mind the check so I left it in, but if you want it to be removed I can double check if it's possible to reach it (and ask AI). Personally I don't think an extra if statement, especially one on a rare path is bad (rare because it happens on the first read, and 16k after that)

[artemananiev]

Is it different from readVarInt(true)? If so, this method is completely redundant, just bloats the API

[ldintr]

It's the same but I was able to measure a 1% improvement by having a version that doesn't use an if statement. I made many changes at once in the 13th item and measured it. (idk why gh shows 12 for some of these) #919 if you really want to we can rerun the measurements but I believe we ran them 3 times already

[artemananiev]

Here are my benchmarks:

@Benchmark
    public void testReadVarLongBoolean(final Blackhole blackhole) {
        long sum = 0;
        try (final PbjReader in = new PbjReader(array)) {
            for (int i = 0; i < size; i++) {
                if (i % 2 == 0) {
                    sum += in.readVarLong(true);
                } else {
                    sum += in.readVarLong(false);
                }
            }
        }
        blackhole.consume(sum);
    }

    @Benchmark
    public void testReadVarLongZZ(final Blackhole blackhole) {
        long sum = 0;
        try (final PbjReader in = new PbjReader(array)) {
            for (int i = 0; i < size; i++) {
                if (i % 2 == 0) {
                    sum += in.readVarLongZZ();
                } else {
                    sum += in.readVarLongNoZZ();
                }
            }
        }
        blackhole.consume(sum);
    }

And here is the result:

Benchmark                                      (size)   Mode  Cnt      Score      Error  Units
PbjReaderReadVarLongZZ.testReadVarLongBoolean   10000  thrpt    5  28512.090 ± 2278.525  ops/s
PbjReaderReadVarLongZZ.testReadVarLongZZ        10000  thrpt    5  28345.451 ± 3432.496  ops/s

[artemananiev]

Should it really be public?

[ldintr]

Yes. parseNoEx (that gets introduce in codec in "Switched to PbjReader/PbjWriter" hash 71e5a9d) returns null on error and error code is needed to understand what the error is, especially if trying to avoid exceptions (the throwOnError alternative).