-
Notifications
You must be signed in to change notification settings - Fork 37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replace protobuf-java(lite) with pure Kotlin implementations on JVM #148
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 lmk if I should review the files with the license at the top, since those are meaty.
runtime/src/commonJvmAndroid/kotlin/pbandk/internal/binary/InputStreamWireReader.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! I must admit that my knowledge of the project is still a bit limited to understand all changes in depth, but I tried to leave some meaningful comments.
Thanks also for the clear performance data! Honestly. I'm not too worried about that, because:
- protobuf most likely used in conjunction with some kind of network requests, so we should ensure "serialization/deserialization time" <<< "network request time" instead of focussing on absolute times
- time per serialization/deserialization is still low: if I understand the data correctly, the entire conformance suite runs in under 2 sec? How many tests are there in the suite? Assuming it's a 1000, then we are looking at ~2ms, which is below a 60fps frame rendering time.
- there is a way to optimize this further (using streams) which we can explain in the readme OR we can ensure that the default examples use the fast method (e.g. provide a retrofit convertor based on streams)
runtime/src/commonJvmAndroid/kotlin/pbandk/internal/binary/BinaryMessageDecoderJvm.kt
Show resolved
Hide resolved
runtime/src/commonJvmAndroid/kotlin/pbandk/internal/binary/BinaryMessageDecoderJvm.kt
Show resolved
Hide resolved
Since protobuf messages are not self-delimiting, by default `decodeFromStream()` will try to read until the end of the stream and try to decode all bytes it reads as part of the message. Applications will often prefix a message with its length when writing multiple messages to a single output stream. When consuming such a stream, the application can read the length first and then pass it to `decodeFromStream()` to make sure only that many bytes are read from the stream.
The conformance test communicates with the conformance test runner using protocol buffer messages over stdin/stdout. By default it uses `encodeToByteArray()` and `decodeFromByteArray()` to encode/decode the messages on all platforms. By setting the `PBANDK_CONFORMANCE_JVM_IO` environment variable to either `BYTE_BUFFER` or `BYTE_BUFFER`, the conformance tests will instead encode/decode using `ByteBuffer` or `InputStream`/`OutputStream` on the JVM. This is handy as a quick test that those I/O paths work correctly. It's also handy as a rough benchmark since the conformance test involves a fair amount of protocol buffer encoding/decoding.
This avoids compatibility issues when applications have transitive dependencies on different versions of protobuf-java(lite) via pbandk and some other library (e.g. Firebase). Also modify the `encodeToStream()` method to return the number of bytes that were written to the stream. This can be useful information for the caller.
Since we no longer depend on protobuf-java(lite), we now have to bundle these proto files ourselves. Applications using the Protobuf Gradle Plugin expect these proto files to be available in pbandk (or one of its dependencies) in order to run `protoc-gen-kotlin`.
…d proto files Now that we're bundling the well-known type proto files, we no longer need to read them from the copy of protobuf installed on the build system. This update also pulled in a newer version of `descriptor.proto` with an added field.
This avoids compatibility issues when applications have transitive dependencies on different versions of protobuf-java(lite) via pbandk and some other library (e.g. Firebase). Since we no longer depend on protobuf-java(lite), we now also bundle
the well-known types proto files ourselves. Applications using the Protobuf Gradle Plugin expect these proto files to be available in pbandk (or one of its dependencies) in order to run
protoc-gen-kotlin
.Additional changes included in this PR:
Allow reading multiple messages from an InputStream
Since protobuf messages are not self-delimiting, by default
decodeFromStream()
will try to read until the end of the stream and try to decode all bytes it reads as part of the message. Applications will often prefix a message with its length when writing multiple messages to a single output stream. When consuming such a stream, the application can read the length first and then pass it todecodeFromStream()
to make sure only that many bytes are read from the stream. Also modify theencodeToStream()
method to return the number of bytes that were written to the stream.Allow running JVM conformance tests with different I/O implementations
By setting the
PBANDK_CONFORMANCE_JVM_IO
environment variable to eitherBYTE_BUFFER
orBYTE_BUFFER
, the conformance tests will instead encode/decode usingByteBuffer
orInputStream
/OutputStream
on the JVM. This is handy as a quick test that those I/O paths work correctly. It's also handy as a rough benchmark since the conformance test involves a fair amount of protocol buffer encoding/decoding.I did some very rough benchmarks of this change by running the conformance tests using both the old
protobuf-java
and the new pure-Kotlin implementations. From my benchmarks, the pure-Kotlin implementation is 70% slower than theprotobuf-java
implementation when encoding/decoding usingByteArray
s orByteBuffer
s. The pure-Kotlin implementation is comparable in speed (might even be slightly faster) when encoding/decoding usingInputStream
/OutputStream
. This isn't completely surprising since theprotobuf-java
library has specialized implementations that make use ofsun.misc.Unsafe
for faster access to the byte array whensun.misc.Unsafe
is available, whereas our pure-Kotlin implementation is only using the officialByteArray
APIs.This benchmark was running under the OpenJDK JVM on MacOS. Results might or might not be different on Android (since the ART runtime is very different than a typical desktop JVM, and the
protobuf-javalite
library also is implemented differently fromprotobuf-java
) but I don't have an easy way to run the benchmarks on an Android device. The conformance test runner communicates with pbandk using protocol buffers sent over stdin/stdout. I modified the pbandk conformance test code to allow choosing whether to perform that stdin/stdout communication using eitherByteArray
,ByteBuffer
, orInputStream
/OutputStream
.These are the results with the previous
protobuf-java
pbandk implementation:and these are the results with the new pure-Kotlin pbandk implementation: