pjfanning commented on PR #862:
URL: https://github.com/apache/pekko-grpc/pull/862#issuecomment-5408669395
Overall this is a good direction — extending the benchmark suite to all four
RPC styles across both DSLs is a genuine improvement, and the `FastFuture` work
in `unmarshal` is a real win (the ~5x jump for pre-2.0 generated handlers is
the standout result). A few things I'd like to see addressed before merge, plus
one API change I'd push back on.
### Correctness
**1. `handleUnary` silently drops the caller's `eHandler` when marshalling
fails** —
`runtime/src/main/scala/org/apache/pekko/grpc/scaladsl/GrpcMarshalling.scala`
```scala
.map(e => marshal[Out](e))(ExecutionContext.parasitic)
```
`marshal[T](e, eHandler = GrpcExceptionHandler.defaultMapper)`, so the
second parameter defaults here, and `marshal` catches `NonFatal` internally and
maps it with `defaultMapper`. The code being replaced used `marshal[Out](out,
eHandler)`, and the streaming branch of the generated handler still uses
`@{method.marshal}(e, eHandler)`. Net effect: a service registered with a
custom `eHandler` gets default trailers/status for any serialization failure on
unary calls, inconsistently with every other method type. Should be
`marshal[Out](e, eHandler)`.
**2. `GoogleProtobufSerializer.serialize(t, frame, offset)` ignores
`offset`** —
`runtime/src/main/scala/org/apache/pekko/grpc/javadsl/GoogleProtobufSerializer.scala`
```scala
override private[grpc] def serialize(t: T, frame: Array[Byte], offset: Int):
Unit = {
val dataLength = t.getSerializedSize
val output = CodedOutputStream.newInstance(frame,
AbstractGrpcProtocol.FrameHeaderSize, dataLength)
```
The `ScalapbProtobufSerializer` counterpart correctly uses `offset`. It
happens to work today only because the sole caller passes `FrameHeaderSize`;
any other offset writes to the wrong place. Latent silent corruption — worth
fixing even though nothing hits it yet.
### API / binary compatibility
**3. Moving the frame-serialization methods onto public `ProtobufSerializer`
is a source and binary break.**
`ProtobufSerializer` is public and carries no
`@InternalApi`/`@ApiMayChange`. Adding two abstract `private[grpc]` members
means nobody outside `org.apache.pekko.grpc` can implement the trait any more —
they can't name, let alone override, a `private[grpc]` member. Verified on
2.13.16 with a minimal reproduction:
```
class MySer needs to be abstract.
Missing implementations for 2 members of trait ProtobufSerializer.
private[package grpc] def serialize(t: String, frame: Array[Byte], offset:
Int): Unit
private[package grpc] def serializedSize(t: String): Int
```
The `private[grpc] trait ProtobufFrameSerializer` this PR deletes existed
precisely to keep the public trait implementable. I'd keep that split: leave
`ProtobufSerializer` alone, put `serializedSize`/`serialize(t, frame, offset)`
on `ProtobufFrameSerializer`, and have `ElementFrame` encoding fall back to
`serialize(t)` + `encodeFrameData` when the serializer isn't a frame serializer.
Related: against the `1.0.2` MiMa baseline I'd also expect flags for the
removal of `GrpcProtocolWriter.frameEncoder` and the `encodeDataToResponse`
signature change (`ByteString` -> `ElementFrame[?]`), plus
`copy`/`apply`/`this` on that case class. `GrpcProtocol` is `@InternalApi` but
MiMa doesn't read the annotation, and there's nothing covering it in
`runtime/src/main/mima-filters/`. Worth confirming `sbt
+mimaReportBinaryIssues` comes back clean, with excludes added where the break
really is internal-only.
### Benchmark numbers
**4. `GrpcMarshallingBenchmark.handleUnaryStrict` regressed 53.4M -> 23.0M
ops/s (-57%)** — that's a 2.3x slowdown on exactly the path #686 optimised,
measured with the same new benchmark against both trees. Calling the
`GrpcMarshallingBenchmark` dips "probably too fine grained" undersells it a
bit. The honest framing is: ~43ns vs ~19ns on an operation that sits inside a
~2us end-to-end handler, so it's lost in the noise at the handler level (and
`generatedUnaryStrict` did go 509k -> 533k). That's a defensible trade for the
code deletion — but I'd state it in the PR body rather than wave it away, so
whoever reads these numbers next understands what was traded.
**5. `marshallStream` regressed 873k -> 793k (-9%)**, and there's a
plausible cause in `GrpcEntityHelpers.chunks`:
```scala
e.map { msg => ElementFrame(msg) }.map(writer.encodeFrame).via(...)
```
Two stream stages where one would do, plus an `ElementFrame` allocation per
element. `e.map(msg => writer.encodeFrame(ElementFrame(msg)))` collapses the
stage; avoiding the wrapper allocation entirely would need something like
`encodeElement[T](t)(implicit ser)` on the writer.
### Smaller things
- **No new tests.** The only runtime test change is a deletion
(`GrpcMarshallingSpec` "recover a failed unary response stage"), fair enough
since `handleUnaryResponse` is gone — but nothing replaces it. Given (1)
slipped through unnoticed, a test asserting that a custom `eHandler` maps a
marshalling failure on a unary call would pay for itself.
- **`benchmarks/src/main/protobuf/helloworld.proto`** is a verbatim copy of
the plugin-tester proto, `//#walkthrough` / `//#options` paradox snippet
markers and all. Those markers are meaningless here and the copy will drift.
Worth at least stripping them, or sharing the source.
- **Wildcard imports**: the new benchmark files use `import
pekko.http.scaladsl.model.*` / `pekko.http.javadsl.model.*`. Project convention
elsewhere is `._`. Cosmetic, but easy to align.
- **`@inline` sprinkling**: the build does enable the Scala 2 inliner so
these aren't pure no-ops on 2.13, but they do nothing on Scala 3, and several
are on methods taking function parameters or overriding abstract members, where
the call site can't resolve a concrete target anyway. I'd keep only the ones
you can tie to a number.
- **`BenchRunner`** loses the sorted summary report it used to print —
that's a UX change for anyone running `benchmarks/run` directly. Intentional?
- **`ByteStringOutputStream.toByteStringUnsafe`** hands out a `ByteString`
wrapping the still-live `ByteArrayOutputStream` buffer. Safe at both current
call sites (the stream is dead afterwards), but it's a general-purpose
`private[grpc]` utility now — worth a scaladoc line saying the stream must not
be written to afterwards.
- Naming: `ElementFrame`'s parameter is still called `data` in
`encodeDataToResponse`/`encodeDataToFrameBytes` even though it's now a frame,
and the `serialize(t)` / `serialize(t, frame, offset)` overload pair reads
ambiguously — `serializeTo` would be clearer.
### Design note
Putting a `ProtobufSerializer` inside a `Frame` moves protobuf knowledge
into the protocol writer, which had been a purely byte-oriented layer. It does
buy the single-allocation encode across all protocol variants and codecs, which
is the point — but I'd frame it as trading one leak for another rather than as
strictly better encapsulation, and it's the direct cause of (3). Scoping the
new methods to `ProtobufFrameSerializer` keeps the win without the public-API
cost.
Happy to see this land once (1), (2) and the MiMa story are sorted. (3) is
the one I'd like a second opinion on, but my preference is to keep
`ProtobufFrameSerializer`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
