perf: Remove interface#836
Conversation
Signed-off-by: ldintr <levo.d@swirldslabs.com>
Signed-off-by: ldintr <levo.d@swirldslabs.com>
Signed-off-by: ldintr <levo.d@swirldslabs.com>
JUnit Test Report 521 files ±0 521 suites ±0 29s ⏱️ -1s Results for commit 82b64da. ± Comparison against base commit 03830e4. This pull request removes 6 and adds 6 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Integration Test Report 426 files ±0 426 suites ±0 24m 41s ⏱️ + 5m 6s Results for commit 82b64da. ± Comparison against base commit 03830e4. This pull request removes 2 and adds 2 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
…erytime Signed-off-by: ldintr <levo.d@swirldslabs.com>
| while (input.hasRemaining()) { | ||
| // Note: ReadableStreamingData.hasRemaining() won't flip to false | ||
| while (input.hasMore()) { | ||
| // Note: ReadableStreamingData.hasMore() won't flip to false |
There was a problem hiding this comment.
Minor: the type name in the comment text should probably be updated.
| byte[] buf = new byte[16 << 10]; | ||
| int pos, end; | ||
| int relLimit; | ||
| long absoluteLimit, offset; | ||
| boolean seenEOF; | ||
| private ReadableSequentialData input; |
There was a problem hiding this comment.
All the fields should be private.
| } | ||
|
|
||
| public SlimBuffer(byte[] buf) { | ||
| this.buf = buf; |
There was a problem hiding this comment.
You want to remove the default initializer from line 12 and move it into a constructor that needs that array initialized. Otherwise, when using this specific constructor at line 23, a new object would allocate a 16K array and then immediately throw it away at this line 24, which is bad for GC and performance in general.
| import java.nio.BufferUnderflowException; | ||
| import java.nio.ByteBuffer; | ||
|
|
||
| public class SlimBuffer { |
There was a problem hiding this comment.
Please add some javadoc for every public class/method in here. You can use the short syntax with ///, or the long one with /** ... */.
| } | ||
|
|
||
| private void grow(int minCap) { | ||
| int newCap = Math.max(buf.length * 2, 1 << 63 - Long.numberOfLeadingZeros((minCap << 1) - 1)); |
There was a problem hiding this comment.
Please add parentheses around the 63 - Long... expression. I agree that Java/javac don't need them. But humans who read this code do.
| while (true) { | ||
| if (pos < relLimit) return true; | ||
| if (seenEOF || relLimit < end) return false; | ||
| bufferMore(); |
There was a problem hiding this comment.
Why does this need to happen in a while(true) loop? Or to rephrase, why do we want to be able to call the bufferMore() more than once? Can we rewrite this code to make it clear that this method calls bufferMore() at most once (aka, can we nuke the while(true))?
| throw new IllegalArgumentException(); | ||
| } | ||
| if (pos + amount <= relLimit) return; | ||
| if (seenEOF) throw new BufferUnderflowException(); |
There was a problem hiding this comment.
Again, this will always throw for an object constructed via SlimBuffer(byte[]). How is this constructor useful?
| if (!seenEOF) { | ||
| for (int i = 0; !seenEOF && i < 32; i++) { | ||
| bufferMore(); | ||
| } | ||
| if (seenEOF == false) { | ||
| throw new RuntimeException(); // logic error |
There was a problem hiding this comment.
I'm not sure I understand why this is an error.
I don't understand why the bufferMore() called 32 times is assumed to guarantee to read the entire input stream?
Also, I'm unsure why the limit() getter method is supposed to perform I/O.
Also, I'm unsure why it's okay to read the entire input in memory when it may be very huge. For example, some block objects may occupy over 32MB. We don't want to read them all into memory when trying to parse them.
| public long readVarLong(final boolean zigZag) throws BufferUnderflowException, UncheckedIOException { | ||
| long value = 0; | ||
| for (int i = 0; i < 10; i++) { | ||
| final byte b = readByte(); | ||
| value |= (long) (b & 0x7F) << (i * 7); | ||
| if (b >= 0) { | ||
| return zigZag ? (value >>> 1) ^ -(value & 1) : value; | ||
| } | ||
| } | ||
| throw new DataEncodingException("Malformed var int"); | ||
| } |
There was a problem hiding this comment.
This is an old algorithm. We replaced it recently at #826
You want to implement the new algorithm here.
| int maxDepth, | ||
| int maxSize) | ||
| throws ParseException { | ||
| return parse(new SlimBuffer(input), strictMode, parseUnknownFields, maxDepth, maxSize); |
There was a problem hiding this comment.
This will create a new SlimBuffer object for every parse() call in all the existing code in Consensus and Block node. I'm afraid that this may degrade the performance. And I see that you've modified the GenericParserQuickBench to use SlimBuffers natively, which hides this potential performance degradation.
I suggest building a benchmark (or reverting the changes in the aforementioned one) that calls the parse(ReadableSequentialData... at all times, and then run it on the main branch and on the branch of this PR. The numbers would give us an idea of how much the performance of the CN and BN would degrade.
If the degradation looks reasonable in the test (say, at most 5% - but I'll let @OlegMazurov and @alex-kuzmin-hg provide their opinion), then we could proceed with releasing this change and then benchmarking the CN and BN code using our performance team tools before actually upgrading PBJ in those systems.
An alternative approach, and/or if the performance degradation is significant, would be to modify all the code in CN and BN to switch to using the SlimBuffer before upgrading PBJ.
Yet another alternative is to keep existing parse(ReadableSequentialData) implementations as they are and introduce the new parse(SlimBuffer) as an extra API. This latter approach would simplify the migration of the CN and BN code as it could be executed step by step migrating one piece of code at a time.
In general, I'm extremely concerned about this particular change here at this line (and all lines below that create new instances of SlimBuffer silently), and I'd suggest to ask our performance engineers to take a look at this.
anthony-swirldslabs
requested review from
OlegMazurov,
alex-kuzmin-hg,
artemananiev and
jasperpotts
2 participants
Description:
The first commit is easier to go through, touches a lot of signature. The rest can be squashed
Bench:
Current:
Benchmark (type) Mode Cnt Score Error Units
GenericParserQuickBench.bench NotCacheableAccountIDType thrpt 15 458.108 ± 2.490 ops/us
https://github.com/hashgraph/pbj/actions/runs/26190721867/job/77058132411
main:
Benchmark (type) Mode Cnt Score Error Units
GenericParserQuickBench.bench NotCacheableAccountIDType thrpt 15 458.108 ± 2.490 ops/us
https://github.com/hashgraph/pbj/actions/runs/26180014678/job/77020208498
Checklist