Use Unsafe memory buffer instead of byte buffer - https://github.com/imprint-serde/imprint-java/issues/21#24
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
…ome micro-optimizations added that were found along the way
* 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
# Conflicts: # build.gradle # src/jmh/java/com/imprint/benchmark/ComparisonBenchmark.java # src/jmh/java/com/imprint/benchmark/ImprintDetailedBenchmark.java # src/jmh/java/com/imprint/benchmark/serializers/AvroSerializingBenchmark.java # src/jmh/java/com/imprint/benchmark/serializers/ImprintSerializingBenchmark.java # src/main/java/com/imprint/core/ImprintFieldObjectMap.java # src/main/java/com/imprint/core/ImprintRecord.java # src/main/java/com/imprint/core/ImprintRecordBuilder.java # src/main/java/com/imprint/ops/ImprintOperations.java # src/main/java/com/imprint/types/ImprintDeserializers.java # src/main/java/com/imprint/types/ImprintSerializers.java # src/main/java/com/imprint/types/MapKey.java # src/main/java/com/imprint/types/TypeCode.java # src/main/java/com/imprint/util/VarInt.java # src/test/java/com/imprint/ops/ImprintOperationsTest.java # src/test/java/com/imprint/profile/ProfilerTest.java
| jmhImplementation 'org.msgpack:jackson-dataformat-msgpack:0.9.8' | ||
| jmhImplementation 'org.apache.thrift:libthrift:0.19.0' | ||
| jmhImplementation 'javax.annotation:javax.annotation-api:1.3.2' | ||
| jmhImplementation 'net.openhft:chronicle-wire:2.25ea5' |
There was a problem hiding this comment.
Got frustrated halfway through the effort and decided to add ChronicleWire as a comparison
| @Fork(value = 1, jvmArgs = {"-Xms4g", "-Xmx4g"}) | ||
| @Measurement(iterations = 10, time = 1) | ||
| @Fork(value = 1, jvmArgs = {"-Xms4g", "-Xmx4g", | ||
| "--illegal-access=permit", |
There was a problem hiding this comment.
ChronicleWire apparently needs all this
|
|
||
| /** | ||
| * Specialized short→object map optimized for ImprintRecordBuilder field IDs. | ||
| * Basically a copy of EclipseCollections's primitive map: |
There was a problem hiding this comment.
Need to clean up javadocs everywhere still
| @ToString(of = {"header"}) | ||
| public class ImprintRecord { | ||
| ByteBuffer serializedBytes; | ||
| ImprintBuffer serializedBytes; |
There was a problem hiding this comment.
Fortunately I was able to get the unsafe buffer to just be a drop in replacement for ByteBuffer so there's no functional changes here in the ImprintRecord
| return this; | ||
| } | ||
|
|
||
| // Builder utilities |
| // 2. Sort fields by ID for directory ordering (zero allocation) | ||
| // 2. Sort fields by ID for directory ordering | ||
| var sortedFieldsResult = getSortedFieldsResult(); | ||
| var sortedValues = sortedFieldsResult.values; |
There was a problem hiding this comment.
This whole block I removed was hacky anyways but also unneeded - the ImprintBuffer is auto-expandable now and handles capacity increases so we don't have to worry too much about it
| @@ -4,375 +4,393 @@ | |||
| import com.imprint.core.*; | |||
There was a problem hiding this comment.
This was difficult but algoritmically everything is the same. I also decided to make merge and project their own static sub-class and separate out some other stuff to Core mostly as an effort to try to organize things (I still need to clean this up eventually since there's a lot of duplication). As I said though - algorithmically nothing has changed here
| @@ -2,11 +2,14 @@ | |||
|
|
|||
There was a problem hiding this comment.
No change here really - just drop in replacement
|
|
||
| // Rough size estimate since actual takes time; might be able to accomodate this better with a growable buffer though | ||
|
|
||
| public static int estimateSize(TypeCode typeCode, Object value) { |
There was a problem hiding this comment.
Debating if I even need this size estimate now since the ImprintBuffer is auto-growable. Can just remove it later if I feel like it
| @@ -0,0 +1,605 @@ | |||
| package com.imprint.util; | |||
There was a problem hiding this comment.
Mostly borrowed concepts from Agrona and Fury for this
|
@expanded-for-real sorry for the delay in reviews, pretty busy at work -- will take a look at this this week! |
agavra
left a comment
There was a problem hiding this comment.
I looked at the production code (particularly the changes to ImprintRecordBuilder and the new ImprintBuffer) and the strategy looks good to me! Since the PR is a little big let me know if there's an area of the code you'd like me to focus a review on.
#21
This was both easier and harder than I originally expected but finally got their. Uses the Unsafe memory buffer largely borrowed from Agrona/Fury to avoid things like boundary checking. This seems standard in other serialization libraries so was ultimately worth it.
Some to-dos - really need to consolidate some of the code better. There's a lot of duplication especially in how we parse and serialize headers.
Also this should pretty much be the end branch that resulted from constant rebasing so future MRs should look a little tidier.