ProtoReader32, a specialization for Kotlin/JS #3077

swankjesse · 2024-08-19T22:15:51Z

No description provided.

wire-runtime/src/commonMain/kotlin/com/squareup/wire/ProtoAdapter.kt

swankjesse · 2024-08-20T02:06:25Z

Next steps:

Rename to ProtoReader32
Make it an interface with factory functions like fun ProtoReader32(ByteArray, Int, Int)
Create a function ProtoReader32.asProtoReader()
Make the base implementation of ProtoAdapter.decode(ProtoReader32) delegate by calling asProtoReader()
Make it opt-in to generate decode() functions that accept a ProtoReader32
Test all combinations of read functions

swankjesse · 2024-08-20T19:49:04Z

wire-runtime/api/wire-runtime.api

+	public abstract fun skip ()V
+}
+
+public final class com/squareup/wire/ProtoReader32Kt {


I don’t love how this API shows up to Java users

swankjesse · 2024-08-20T19:49:47Z

wire-runtime/src/commonMain/kotlin/com/squareup/wire/ByteArrayProtoReader32.kt

@@ -0,0 +1,469 @@
+// This class is derived from the CodedInputByteBuffer class in Google's "Nano" Protocol Buffer
+// implementation. The original copyright notice, list of conditions, and disclaimer for those
+// classes is as follows:


Keeping this 'cause it’s copied from a thing that had it

swankjesse · 2024-08-20T19:50:09Z

wire-runtime/src/commonMain/kotlin/com/squareup/wire/ByteArrayProtoReader32.kt

+import okio.EOFException
+import okio.IOException
+
+internal class ByteArrayProtoReader32(


This is mostly a copy of ProtoReader.kt.

swankjesse · 2024-08-20T19:51:21Z

wire-runtime/src/commonMain/kotlin/com/squareup/wire/ByteArrayProtoReader32.kt

+  private val bufferStack = mutableListOf<Buffer>()
+
+  /** Lazily-initialized. */
+  private var protoReader: ProtoReader32AsProtoReader? = null


This variable and the function below are not in ProtoReader. (Should I just use by lazy ? I don’t have an intuition on the runtime cost of that)

Seems fine as-is

swankjesse · 2024-08-20T19:52:29Z

wire-runtime/src/commonMain/kotlin/com/squareup/wire/ProtoReader32.kt

+
+/** Reads each tag, handles it, and returns a byte string with the unknown fields. */
+@JvmName("-forEachTag") // hide from Java
+inline fun ProtoReader32.forEachTag(tagHandler: (Int) -> Any): ByteString {


This is not a member function in ProtoReader32, but it is a member function in ProtoReader

Why wouldn't we want to make it a member function?

Because the new target type is an interface, I think it fits best to implement as an extension function. (I know that we can put implementations in interface functions now, but we’d only be doing that for symmetry).

Also I don’t like the idea of having an inline function that’s polymorphic.

swankjesse · 2024-08-20T19:53:21Z

...jvmKotlinProtoReader32Test/proto-kotlin/com/squareup/wire/protos/kotlin/alltypes/AllTypes.kt

@@ -0,0 +1,4122 @@
+// Code generated by Wire protocol buffer compiler, do not edit.


100% generated code

JakeWharton · 2024-08-20T20:17:57Z

wire-runtime/src/jsMain/kotlin/com/squareup/wire/ProtoAdapter.kt

+  actual open fun decode(reader: ProtoReader32): E {
+    return decode(reader.asProtoReader())
+  }


Is this codepath tested? I saw the new reader being tested and I saw the old reader being tested on a type with the new function. Didn't see the new reader being tested on a type which does not contain the new function, though.

Ooooh I think it might be accidentally tested, but I think explicitly testing it is much better. Done.

wire-runtime/src/commonTest/kotlin/com/squareup/wire/ProtoReader32Test.kt

Egorand · 2024-08-21T14:13:25Z

wire-runtime/src/commonMain/kotlin/com/squareup/wire/ProtoReader32.kt

+
+/** Reads each tag, handles it, and returns a byte string with the unknown fields. */
+@JvmName("-forEachTag") // hide from Java
+inline fun ProtoReader32.forEachTag(tagHandler: (Int) -> Any): ByteString {


Why wouldn't we want to make it a member function?

Egorand · 2024-08-21T14:16:00Z

wire-runtime/src/commonMain/kotlin/com/squareup/wire/ProtoReader.kt

@@ -47,7 +47,17 @@ import okio.IOException
 /**
 * Reads and decodes protocol message fields.
 */
-class ProtoReader(private val source: BufferedSource) {
+open class ProtoReader(private val source: BufferedSource) {


Would it simplify the design if we introduced a new interface, e.g. IProtoReader<Tag>, and have both ProtoReader and ProtoReader32 implement it? Or that would break backwards compat?

I would love it, but the generic Tag would be boxed on every use, potentially at significant runtime cost.

| Package | Type | Package file | Manager | Update | Change | |---|---|---|---|---|---| | [org.jetbrains:annotations](https://github.com/JetBrains/java-annotations) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `26.0.1` -> `26.0.2` | | [io.grpc:grpc-stub](https://github.com/grpc/grpc-java) | dependencies | misk/gradle/libs.versions.toml | gradle | minor | `1.69.1` -> `1.70.0` | | [io.grpc:grpc-protobuf](https://github.com/grpc/grpc-java) | dependencies | misk/gradle/libs.versions.toml | gradle | minor | `1.69.1` -> `1.70.0` | | [io.grpc:grpc-netty](https://github.com/grpc/grpc-java) | dependencies | misk/gradle/libs.versions.toml | gradle | minor | `1.69.1` -> `1.70.0` | | [io.grpc:protoc-gen-grpc-java](https://github.com/grpc/grpc-java) | dependencies | misk/gradle/libs.versions.toml | gradle | minor | `1.69.1` -> `1.70.0` | | [io.grpc:grpc-bom](https://github.com/grpc/grpc-java) | dependencies | misk/gradle/libs.versions.toml | gradle | minor | `1.69.1` -> `1.70.0` | | [io.grpc:grpc-api](https://github.com/grpc/grpc-java) | dependencies | misk/gradle/libs.versions.toml | gradle | minor | `1.69.1` -> `1.70.0` | | [com.google.api-client:google-api-client](https://github.com/googleapis/google-api-java-client) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.7.1` -> `2.7.2` | | [com.squareup.wire:wire-schema](https://github.com/square/wire) | dependencies | misk/gradle/libs.versions.toml | gradle | minor | `5.0.0` -> `5.2.1` | | [com.squareup.wire:wire-runtime](https://github.com/square/wire) | dependencies | misk/gradle/libs.versions.toml | gradle | minor | `5.0.0` -> `5.2.1` | | [com.squareup.wire:wire-reflector](https://github.com/square/wire) | dependencies | misk/gradle/libs.versions.toml | gradle | minor | `5.0.0` -> `5.2.1` | | [com.squareup.wire:wire-moshi-adapter](https://github.com/square/wire) | dependencies | misk/gradle/libs.versions.toml | gradle | minor | `5.0.0` -> `5.2.1` | | [com.squareup.wire:wire-grpc-client](https://github.com/square/wire) | dependencies | misk/gradle/libs.versions.toml | gradle | minor | `5.0.0` -> `5.2.1` | | [com.squareup.wire:wire-gradle-plugin](https://github.com/square/wire) | dependencies | misk/gradle/libs.versions.toml | gradle | minor | `5.0.0` -> `5.2.1` | | [com.squareup.wire:wire-bom](https://github.com/square/wire) | dependencies | misk/gradle/libs.versions.toml | gradle | minor | `5.0.0` -> `5.2.1` | | [com.google.auth:google-auth-library-oauth2-http](https://github.com/googleapis/google-auth-library-java) | dependencies | misk/gradle/libs.versions.toml | gradle | minor | `1.30.1` -> `1.31.0` | | [com.google.auth:google-auth-library-credentials](https://github.com/googleapis/google-auth-library-java) | dependencies | misk/gradle/libs.versions.toml | gradle | minor | `1.30.1` -> `1.31.0` | | [com.datadoghq:dd-trace-api](https://github.com/datadog/dd-trace-java) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `1.45.1` -> `1.45.2` | | [com.datadoghq:dd-trace-ot](https://github.com/datadog/dd-trace-java) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `1.45.1` -> `1.45.2` | | [software.amazon.awssdk:sdk-core](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.30.2` -> `2.30.4` | | [software.amazon.awssdk:dynamodb-enhanced](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.30.2` -> `2.30.4` | | [software.amazon.awssdk:dynamodb](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.30.2` -> `2.30.4` | | [software.amazon.awssdk:aws-core](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.30.2` -> `2.30.4` | | [software.amazon.awssdk:bom](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.30.2` -> `2.30.4` | | [software.amazon.awssdk:auth](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.30.2` -> `2.30.4` | --- ### Release Notes <details> <summary>JetBrains/java-annotations (org.jetbrains:annotations)</summary> ### [`v26.0.2`](https://github.com/JetBrains/java-annotations/blob/HEAD/CHANGELOG.md#Version-2602) [Compare Source](JetBrains/java-annotations@26.0.1...26.0.2) - Fixed missing klibs for apple artifacts. </details> <details> <summary>googleapis/google-api-java-client (com.google.api-client:google-api-client)</summary> ### [`v2.7.2`](https://github.com/googleapis/google-api-java-client/blob/HEAD/CHANGELOG.md#272-2025-01-22) ##### Bug Fixes - Add warnings to users about using credentials from external sources ([#2551](googleapis/google-api-java-client#2551)) ([3bb2879](googleapis/google-api-java-client@3bb2879)) </details> <details> <summary>square/wire (com.squareup.wire:wire-schema)</summary> ### [`v5.2.1`](https://github.com/square/wire/blob/HEAD/CHANGELOG.md#Version-521) [Compare Source](square/wire@5.2.0...5.2.1) *2025-01-07* ##### JVM generation - Fix support for mutable messages in Wire's Kotlin Generator. ([#3233](square/wire#3233) by \[Rahul Ravikumar]\[tikurahul]) ### [`v5.2.0`](https://github.com/square/wire/blob/HEAD/CHANGELOG.md#Version-520) [Compare Source](square/wire@5.1.0...5.2.0) *2025-01-06* ##### Common - Enforce recursion limit when parsing nested groups. ([#3119](square/wire#3119)) ##### CLI `wire-compiler` - It is now possible to set multiple targets. ([#3106](square/wire#3106) & [#3107](square/wire#3107)) - The option `opaque_types` introduced in `4.9.2` for the Wire Gradle plugin is now available on CLI. ([#3147](square/wire#3147)) ##### JVM generation - [KotlinPoet has been updated to `2.0.0`](https://square.github.io/kotlinpoet/changelog/#version-200) which dramatically changes how generated Kotlin files are wrapped. This is neither a source nor a binary breaking changes. - A new `@WireEnclosingType` annotation is now applied to generated types so R8 doesn't prune too much. ([#3123](square/wire#3123)) - Split the redact method into chunks when a type has more than 100 fields to avoid compilation error. ([#3214](square/wire#3214) by \[Damian Wieczorek]\[damianw]) - Add support for mutable messages in Wire's Kotlin Generator. ([#3217](square/wire#3217) by \[Rahul Ravikumar]\[tikurahul]) - You can opt-in by adding `mutableTypes = true` on your Kotlin target. This is unsafe and we do not recommend that you use it unless you have a sound use-case for it. - Wire is now using Palantir's JavaPoet instead of Square's JavaPoet. ##### Swift - Fix buffer overflow and data corruption when a type has more than 5 layers of nesting ([#3203](square/wire#3203) by \[Eric Amorde]\[amorde]) ### [`v5.1.0`](https://github.com/square/wire/blob/HEAD/CHANGELOG.md#Version-510) [Compare Source](square/wire@5.0.0...5.1.0) *2024-09-11* ##### Common - Support for Kotlin `2.0.20`. ([#3093](square/wire#3093)) - `srcDir(String)` has been undeprecated. ([#3039](square/wire#3039)) - Some loggings now happen at the debug level, instead of info. ([#3041](square/wire#3041)) - Remove some unactionable warnings on Kotlin/JS ([#3047](square/wire#3047)) - Propagate the deprecated flag on EnumType after pruning by wire-gradle-plugin ([#3076](square/wire#3076) by \[Aaron Edwards]\[aaron-edwards]) - Introduce `ProtoReader32`, a specialization for Kotlin/JS ([#3077](square/wire#3077)) This is an alternative to `ProtoReader`, which uses `Long` as a cursor. It originates as an optimization for Kotlin/JS, where `Long` cursors are prohibitively expensive. - Fix Gradle project isolation issue when reading a property ([#3078](square/wire#3078) by \[Aurimas]\[liutikas]) - Change the recursion limit to match grpc's default ([#3091](square/wire#3091)) ##### Kotlin - New enum option `enum_mode` to take precedence over the `enumMode` option added in `5.0.0-alpha02`. Use this if you want to migrate your enums granularly. ([#2993](square/wire#2993)) - Don't throw if reading trailers fail ([#3087](square/wire#3087)) ##### Swift - Avoid crash when parsing an empty repeated `[packed=true]` for fixed-length types. ([#3044](square/wire#3044) by \[Sasha Weiss]\[sashaweiss-signal]) </details> <details> <summary>googleapis/google-auth-library-java (com.google.auth:google-auth-library-oauth2-http)</summary> ### [`v1.31.0`](https://github.com/googleapis/google-auth-library-java/blob/HEAD/CHANGELOG.md#1310-2025-01-22) ##### Features - ImpersonatedCredentials to support universe domain for idtoken and signblob ([#1566](googleapis/google-auth-library-java#1566)) ([adc2ff3](googleapis/google-auth-library-java@adc2ff3)) - Support transport and binding-enforcement MDS parameters. ([#1558](googleapis/google-auth-library-java#1558)) ([9828a8e](googleapis/google-auth-library-java@9828a8e)) ##### Documentation - Promote use of bill of materials in quickstart documentation ([#1620](googleapis/google-auth-library-java#1620)) ([fc20d9c](googleapis/google-auth-library-java@fc20d9c)), closes [#1552](googleapis/google-auth-library-java#1552) </details> <details> <summary>datadog/dd-trace-java (com.datadoghq:dd-trace-api)</summary> ### [`v1.45.2`](https://github.com/DataDog/dd-trace-java/releases/tag/v1.45.2): 1.45.2 ##### Components ##### Application Security Management (WAF) - 🐛 🍒 8258 - Prevents a NPE when there is no subscriber for user events ([#8260](DataDog/dd-trace-java#8260) - [@manuel-alvarez-alvarez](https://github.com/manuel-alvarez-alvarez)) </details> --- ### Configuration 📅 **Schedule**: Branch creation - "after 6pm every weekday,before 2am every weekday" in timezone Australia/Melbourne, Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 👻 **Immortal**: This PR will be recreated if closed unmerged. Get [config help](https://github.com/renovatebot/renovate/discussions) if that's undesired. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). GitOrigin-RevId: 12db0f59db2e6ebf55203c87fccab042d495106a

swankjesse commented Aug 19, 2024

View reviewed changes

wire-runtime/src/commonMain/kotlin/com/squareup/wire/ProtoAdapter.kt Outdated Show resolved Hide resolved

swankjesse commented Aug 19, 2024

View reviewed changes

wire-runtime/src/commonMain/kotlin/com/squareup/wire/ProtoAdapter.kt Outdated Show resolved Hide resolved

ProtoReader32, a specialization for Kotlin/JS

6884cd1

swankjesse force-pushed the jwilson.0819.ByteArrayProtoReader branch from 09db36c to 6884cd1 Compare August 20, 2024 16:55

swankjesse changed the title ~~ByteArrayProtoReader, a specialization for Kotlin/JS~~ ProtoReader32, a specialization for Kotlin/JS Aug 20, 2024

swankjesse requested review from Egorand and adrw August 20, 2024 16:57

squarejesse added 3 commits August 20, 2024 14:59

Make it opt-in to support ProtoReader32

998de0a

Document emitProtoReader32

7d17a2c

Generate test messages for ProtoReader32

5ffab75

swankjesse marked this pull request as ready for review August 20, 2024 19:40

swankjesse requested review from JakeWharton and removed request for adrw August 20, 2024 19:40

swankjesse commented Aug 20, 2024

View reviewed changes

JakeWharton approved these changes Aug 20, 2024

View reviewed changes

Egorand approved these changes Aug 21, 2024

View reviewed changes

squarejesse added 2 commits August 21, 2024 12:52

Address code review feedback

6590bce

More mix and match tests

60be017

swankjesse merged commit a67d51e into master Aug 21, 2024
11 checks passed

swankjesse deleted the jwilson.0819.ByteArrayProtoReader branch August 21, 2024 18:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ProtoReader32, a specialization for Kotlin/JS #3077

ProtoReader32, a specialization for Kotlin/JS #3077

swankjesse commented Aug 19, 2024 •

edited

Loading

swankjesse commented Aug 20, 2024 •

edited

Loading

swankjesse Aug 20, 2024

swankjesse Aug 20, 2024

swankjesse Aug 20, 2024

swankjesse Aug 20, 2024

JakeWharton Aug 20, 2024

swankjesse Aug 20, 2024

Egorand Aug 21, 2024

swankjesse Aug 21, 2024

swankjesse Aug 21, 2024

swankjesse Aug 20, 2024

JakeWharton Aug 20, 2024

swankjesse Aug 21, 2024

Egorand Aug 21, 2024

Egorand Aug 21, 2024

swankjesse Aug 21, 2024

		@@ -0,0 +1,4122 @@
		// Code generated by Wire protocol buffer compiler, do not edit.

ProtoReader32, a specialization for Kotlin/JS #3077

ProtoReader32, a specialization for Kotlin/JS #3077

Conversation

swankjesse commented Aug 19, 2024 • edited Loading

swankjesse commented Aug 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swankjesse commented Aug 19, 2024 •

edited

Loading

swankjesse commented Aug 20, 2024 •

edited

Loading