Add option to use primitive arrays for packed scalars#2433
Conversation
|
Okay I think this is ready for review. Notably because I can't really use generics here, I've chosen to only support I do still have to put up a PR to reserve the extension for Wire, similar to https://github.com/protocolbuffers/protobuf/pull/7020/files. Was planning on doing that after this is approved (or at least the API), and then before it's merged. In terms of testing, I feel pretty good about the test changes I made. Unfortunately trying to test for autoboxing isn't really feasible, but in terms of correctness we should be good. |
JGulbronson
changed the title
|
Note that this doesn't prevent autoboxing, because of the use of generics in the ProtoAdapter abstract class |
|
Okay this should prevent autoboxing now |
|
The multiplatform test failed because of an HTTP error
|
release notes: no Supports square/wire#2433 Closes #12491 COPYBARA_INTEGRATE_REVIEW=#12491 from JGulbronson:jgulbronson.claim-1180-for-wire f577336 PiperOrigin-RevId: 525793344
| val fieldName = localNameAllocator[fieldOrOneOf] | ||
| addStatement("if (%1L != %2N.%1L) return·false", fieldName, otherName) | ||
| if (fieldOrOneOf.useArray) { | ||
| addStatement("if (!%1L.contentEquals(%2N.%1L)) return·false", fieldName, otherName) |
| fieldName | ||
| ) | ||
| if (field.useArray && reverse) { | ||
| val encodeArray = MemberName("com.squareup.wire.internal", "encodeArray") |
| EncodeMode.REPEATED -> List::class.asClassName().parameterizedBy(baseClass) | ||
| EncodeMode.PACKED -> { | ||
| if (useArray) { | ||
| primitiveArrayClassForType.asTypeName() |
| override fun encodedSize(value: DoubleArray): Int { | ||
| var size = 0 | ||
| for (i in 0 until value.size) { | ||
| size += originalAdapter.encodedSize(value[i]) |
There was a problem hiding this comment.
I don’t know if I prefer this or not:
return originalAdapter.encodedSize(0.0) * value.size
There was a problem hiding this comment.
Possibly not worth the deviation
There was a problem hiding this comment.
That would be optimizing the slow path right? Since using the reverse writer means you just get the size after encoding?
| linker.errors += "packed=true not permitted on $type" | ||
| } | ||
| if (useArray && !isPacked) { | ||
| linker.errors += "wire.use_array=true only permitted on packed fields" |
There was a problem hiding this comment.
Perfect, this is what I was asking for above. Yay!
There was a problem hiding this comment.
Yep, resolving the original comment
| } catch (expected: SchemaException) { | ||
| assertThat(expected).hasMessage( | ||
| """ | ||
| |wire.use_array=true only permitted on packed fields |
- Rename `getTruncatedArray()` to `toArray()` - Add copyright headers - Add helper `forDecoding()` method on companion objects
| override fun toString(): String = data.copyOf(size).contentToString() | ||
|
|
||
| companion object { | ||
| fun forDecoding( |
| .coerceAtMost(Int.MAX_VALUE.toLong()) | ||
| .toInt() | ||
| array_int32 = IntArrayList(initialCapacity) | ||
| array_int32 = IntArrayList.forDecoding(reader.nextFieldMinLengthInBytes(), 1) |
3 participants
This will allow us to generate repeated, packed, scalar fields to use the appropriate Array type. This has huge benefits for the JVM, because it will prevent autoboxing of values. (For example,
float->Float).This is still a WIP, I've largely hardcoded things as
FloatArraywhile I play around with it, but it's starting to take shape. Unfortunately we can't use generics and avoid the autoboxing, hence the 4 different*ArrayListclasses.