Full Comprehensive Comparison Tests#9
Conversation
…mance tracking; add comprehensive String benchmark
…to remove a bunch of stuff
…ing deserialization a bit faster
Try to enhance string deserialization
A full list of enhancements can be found here - #3
* Full comprehensive comparison tests with a lot of other libraries + some micro-optimizations added that were found along the way * replace deprecated gradle methods with latest --------- Co-authored-by: expand3d <>
# Conflicts: # src/jmh/java/com/imprint/benchmark/ComparisonBenchmark.java # src/main/java/com/imprint/core/ImprintRecord.java # src/main/java/com/imprint/types/TypeHandler.java # src/main/java/com/imprint/types/Value.java
There was a problem hiding this comment.
The CI file should trigger a Comprehensive benchmark run on merges to main and add them as a report
There was a problem hiding this comment.
Lots of changes in the build.gradle to add other frameworks we're comparing against; includes protobuf and flatbuffers which requires major gradle work as demonstrated below
| } | ||
| } | ||
|
|
||
| // Download and setup FlatBuffers compiler for Linux (CI environment) |
There was a problem hiding this comment.
We have to download a C based flatbuffer compiler to the CI server in order to run the comparisons during the merge process.
There was a problem hiding this comment.
flatbuffer schema file
There was a problem hiding this comment.
protobuf schema file
| public ImprintRecord(Header header, List<DirectoryEntry> directory, ByteBuffer payload) { | ||
| this.header = Objects.requireNonNull(header, "Header cannot be null"); | ||
| this.directory = List.copyOf(Objects.requireNonNull(directory, "Directory cannot be null")); | ||
| this.directory = Collections.unmodifiableList(Objects.requireNonNull(directory, "Directory cannot be null")); |
There was a problem hiding this comment.
minor performance gain
| private final byte[] value; | ||
|
|
||
| public BytesValue(byte[] value) { | ||
| this.value = value.clone(); // defensive copy |
There was a problem hiding this comment.
Removing debug comment notes to myself
| private volatile String cachedString; // lazy decode | ||
| private volatile String cachedString; | ||
|
|
||
| private static final int THREAD_LOCAL_BUFFER_SIZE = 1024; |
There was a problem hiding this comment.
Tried and true thread local buffer pool for Strings. It doesn't make a huge difference in micro benchmarks of ~5 iterations but the time saved over larger operations can be significant
| Value deserialize(ByteBuffer buffer) throws ImprintException; | ||
| void serialize(Value value, ByteBuffer buffer) throws ImprintException; | ||
| int estimateSize(Value value) throws ImprintException; | ||
| ByteBuffer readValueBytes(ByteBuffer buffer) throws ImprintException; |
There was a problem hiding this comment.
@agavra I looked at the Rust implementation again you were correct, the previous version for readValueBytes was a bit wonky. I had added this originally since I wanted each type to define it but it's not needed anymore since we'll let the ImprintRecord set these boundaries through the Directory, even for nested values which had originally been causing me trouble. So pretty much all of this can be removed from the interface and underlying
There was a problem hiding this comment.
Lot of test setup here between all the frameworks
| bh.consume(result); | ||
| } | ||
|
|
||
| // ===== DESERIALIZATION BENCHMARKS ===== |
There was a problem hiding this comment.
Imprint and Flatbuffers are kind of unfairly compared against the rest of the frameworks on this one since we're not actually deserializing anything and just setting it up, whereas Jackson for instance is deserializing straight to types. I plan on setting up additional fair tests in the next MR
There was a problem hiding this comment.
Imprint was designed to have zero-overhead deserialization and intentionally sacrifices the cost to deserialize an entire record, since it's larger (which happens pretty rarely in data pipelines). For fairness we can (and should) publish the results for full deserialization, but it's important to keep in mind what we care about when it comes to optimizations.
| } | ||
|
|
||
| // ===== FIELD ACCESS BENCHMARKS ===== | ||
| // Tests accessing a single field near the end of a large record |
There was a problem hiding this comment.
These are more accurate, at least in the sense of achieving the same end goal even though Imprint and Flatbuffers are still doing a lot less work
|
|
||
| // ===== HELPER METHODS ===== | ||
|
|
||
| private void setupAvro() { |
There was a problem hiding this comment.
String blocks aren't until Java 15.....
| return messagePackMapper.writeValueAsBytes(data); | ||
| } | ||
|
|
||
| private byte[] serializeWithAvro(TestRecord data) throws Exception { |
There was a problem hiding this comment.
Avro setup is kind of annoying
| return avroReader.read(null, decoder); | ||
| } | ||
|
|
||
| private byte[] serializeWithProtobuf(TestRecord data) { |
There was a problem hiding this comment.
Protobuf setup is more annoying
| return builder.build().toByteArray(); | ||
| } | ||
|
|
||
| private ByteBuffer serializeWithFlatBuffers(TestRecord data) { |
There was a problem hiding this comment.
I don't know if there's an easier way to do this but my God Flatbuffers is impossible to deal with
| } | ||
|
|
||
| //Single allocation instead of duplicate + slice | ||
| var fieldBuffer = payload.duplicate(); |
There was a problem hiding this comment.
realized here on profiling that slice isn't actually needed.
…es in gradle file. Also fix permission issue
Benchmark ResultsBenchmark execution completed but no results file was found. Check the workflow logs for details. |
…Comparison tests iterations manually
📊 Benchmark ResultsBenchmark execution completed but no results file was found. Check the workflow logs for details. |
agavra
left a comment
There was a problem hiding this comment.
This is really cool, thanks for running all these benchmarks!
At first I was concerned when seeing the results of the merge benchmark then I remembered we haven't implemented the merge algorithm in Java yet 😅 let's make sure to do that before we publish the benchmarks in docs anywhere.
I think the other important benchmark that isn't covered here is "project + serialize". Flatbuffers does a really good job if you're projecting fields, but if you need to project it into a smaller record schema you actually have to reserialize everything you're projecting out. (Basically imagine you have record with fields [id, name, company, email, age, ...] and you only care about [id, name, email] so you want to serialize just those three fields and send it to a downstream application.
Re: serialization is expected to be a little slower with Imprint since typically you aren't serializing the entire records within a pipeline, though I am surprised that it's so much slower. That would be worth looking into to see if we're missing anything.
Lastly, note that for the comparison benchmarks the larger the record the better Imprint fairs. So it's actually interesting to test merge/project with varying record sizes (both number of fields as well as size of the fields, in particular the latter).
| - name: Run size comparison benchmarks | ||
| run: | | ||
| ./gradlew jmhRunSizeComparisonBenchmarks | ||
| continue-on-error: true |
There was a problem hiding this comment.
I don't think we should run the comparison benchmarks on CI - they can be run on demand since we don't expect the performance of other systems to change when we commit to Imprint and the Imprint-specific benchmarks should catch regressions (which is the point of running things on each PR).
Not only does this slow down the PR builds, but I think there's some limit to the free GH plan on how many minutes you can run workflows for (though I need to double check that, it may only apply to private repos)
There was a problem hiding this comment.
This makes my life way easier lol. I can reduce it to just a gradle task for ease of use to run locally and remove all the complex custom tasking
| } | ||
|
|
||
| if (latestFile) { | ||
| console.log(`📊 Found benchmark results: ${latestFile}`); |
There was a problem hiding this comment.
I recommend having the minimal amount of scripting inside ci.yml and instead delegate it to a script that we can just run locally. See https://github.com/imprint-serde/imprint/blob/main/scripts/ci_bench.sh for an example, I generate the markdown within the script there and just call that script from the GHA workflow
| bh.consume(result); | ||
| } | ||
|
|
||
| // ===== DESERIALIZATION BENCHMARKS ===== |
There was a problem hiding this comment.
Imprint was designed to have zero-overhead deserialization and intentionally sacrifices the cost to deserialize an entire record, since it's larger (which happens pretty rarely in data pipelines). For fairness we can (and should) publish the results for full deserialization, but it's important to keep in mind what we care about when it comes to optimizations.
| var usedFieldIds = new HashSet<Integer>(); | ||
|
|
||
| // Copy fields from first record (takes precedence) | ||
|
|
There was a problem hiding this comment.
if we're publishing any of these benchmarks we should make sure to actually implement the merge algorithm 😉 if we need to deserialize/reserialize the entire two records that defeats the purpose of imprint
There was a problem hiding this comment.
I'm going to remove the merge comparisons for now. I'll create a task and start merge algo next and can always come back to them
Satisfies most requirements in - #7. Note that I'm excluding Thrift for a later PR since we have enough data now to proceed. Also excluded Blackbird from Jackson for now but can add later.