timw commented on PR #862:
URL: https://github.com/apache/pekko-grpc/pull/862#issuecomment-5564587251
> Another Claude AI review:
> ### Medium
> **1.
`runtime/src/main/scala/org/apache/pekko/grpc/scaladsl/GrpcMarshalling.scala:146`
— the exception handler is now built on every unary request, including the
happy path.**
>
> `GrpcExceptionHandler.from(eHandler(system.classicSystem))` is constructed
unconditionally. Previously the recovery `PartialFunction` was only built on
the error branch — `case Some(Success(out)) => marshalUnaryResponse(...)`
allocated nothing. `from` calls `eHandler(system)`, then
`defaultMapper(system)`, then `.orElse(...).andThen(...)`: 4+ allocations per
successful call.
"was only built on the error branch" isn't strictly true - the exception
handler construction was only avoided in the case of a `Strict` entity, or when
the `unmarshal` and the implementation invocation both produced fulfilled
futures.
This is on that basis, definitely a regression though for the happy path.
>This looks like a plausible cause of the regressions in this PR's own
numbers (GrpcMarshallingBenchmark.handleUnaryStrict 53.4M → 23.0M ops/s,
syntheticUnaryStrictRequestProcessing 29.9M → 24.6M). The PF depends only on
eHandler, system and writer, all fixed per handler, so it can be hoisted out of
the per-request path.
Unfortunately the dependence on the writer means we can't just hoist this
out of the per-request scope - we basically need a lazy instantiation of the
exception handler on the failure path.
The non-unary generated handlers (and the unary handlers before the recent
optimisation work) always incur this cost, so we could solve it for all of the
handler styles.
It's a little awkward to resolve this with the `Future` API - it will either
incur a per-request FastFuture` wrapper on each success, or require a distinct
`GrpcExceptionHandler.from` that produces a `Throwable => HttpResponse`. I'm
leaning towards the latter, and will suggest a fix like that with the recover
pulled into the generated handler so it can apply to all RPC variants
(effectively removing `handleUnary` and reintroducing the older style generated
code with the `.fast` applications that boost speed, and a new `transform`
based recover path that defers exception handler construction).
> **2.
`runtime/src/main/scala/org/apache/pekko/grpc/scaladsl/GrpcMarshalling.scala:145`
— response marshalling moved to `ExecutionContext.parasitic`.**
>
> When the service implementation's `Future` completes asynchronously,
`marshal[Out]` (full protobuf serialization plus response construction) now
runs inline on whatever thread calls `Promise.success` — a JDBC pool thread, an
outbound client's Netty event loop, or an actor's dispatcher thread if the
implementation completes a promise from inside `receive`. It previously ran on
`mat.executionContext`. For large response messages that is real work stolen
from a foreign thread, and it happens while any lock the completer holds is
still held.
This is kind of the point of that change - stealing time from the thread to
avoid a dispatch. I'd concur avoiding this would be safer in the event of large
Protobuf messages.
> The same applies to the `recoverWith` on the next line, which runs
`GrpcExceptionHandler.defaultMapper` — including `Logging(system, …)`
construction — on that thread.
Assuming the parasitic dispatch on the previous map is removed, this is no
longer the case - this would be explicitly stealing time from a thread in the
handler `ExecutionContext` after it completed the `marshal`.
> **3.
`runtime/src/main/scala/org/apache/pekko/grpc/ProtobufSerializer.scala:39` —
the new public extension point exposes an internal type.**
>
> `ProtobufFrameSerializer` is promoted from `private[grpc]` to public
`@ApiMayChange`, but it extends
`GrpcProtocol.DeferredDataFrame.DeferredDataWriter[T]`, which is nested inside
`@InternalApi object GrpcProtocol`, and both abstract members
(`serializedSize`, `serializeTo`) are inherited from that internal type. Anyone
implementing the hook has to reference a type we declare internal and unstable.
If this is meant to be a supported optimisation hook, `DeferredDataWriter`
should probably move out of `GrpcProtocol`, or the two methods be declared
directly on `ProtobufFrameSerializer`.
Yeah, this is annoying - I'd like to avoid ether the `GrpcProtocol` or
`ProtobufSerializer` knowing about each other, and was trying to avoid an
allocation to wrap it at the boundary. Moving `DeferredDataWriter` out into the
public API area would solve the leak I think.
> ### Low
> **4.
`runtime/src/main/scala/org/apache/pekko/grpc/javadsl/GrpcMarshalling.scala:129`
— binary break against 2.0.0-M2 that MiMa will not catch.**
>
> `handleUnaryResponse` and both `handleUnaryFailure` overloads are removed,
and the Java codegen no longer emits calls to them. Java handlers generated by
2.0.0-M2 and re-run against this build will fail with `NoSuchMethodError` at
request time. MiMa misses it because `mimaCompareVersion` is `1.0.2`
(`build.sbt:102`) and these methods did not exist in 1.0.2.
Milestone-to-milestone this is probably acceptable, but it is worth a changelog
note since it forces regeneration.
I had a search and didn't see a way to update the changelog.
> **5.
`runtime/src/main/scala/org/apache/pekko/grpc/internal/GrpcProtocolNative.scala:71`
— the parameter widening loses an invariant.**
>
> `encodeDataToResponse` now takes an `OutboundFrame` and derives the body
via `encodeFrame(codec, frame).data`. `TrailerFrame` is an `OutboundFrame`, and
for it `encodeFrame` returns `LastChunk`, whose `.data` is `ByteString.empty` —
so passing a trailer frame silently produces a 200 response with an empty body
and no trailers rather than failing. The old `ByteString` parameter made that
unrepresentable. `GrpcProtocolWeb.scala:51` has the mirror problem: a
`TrailerFrame` would be encoded as a trailer frame _plus_ the appended trailer
frame. Narrowing the parameter to a data-frame type, or matching and rejecting
`TrailerFrame`, would restore the guarantee.
This can be solved by introducing `OutputDataFrame` into the sealed trait
hierarchy - which coincidentally then matches some of the method names quite
nicely.
> **6. Leftover debug output in the two new tests.**
>
>
`runtime/src/test/scala/org/apache/pekko/grpc/javadsl/GoogleProtobufSerializerSpec.scala:41`
and
`runtime/src/test/scala/org/apache/pekko/grpc/scaladsl/ScalapbProtobufSerializerSpec.scala:42`
still have `println(serialized.mkString(","))` /
`println(frame.mkString(","))` in the "serialize into buffer at offset" tests.
They fire on every `runtime/test` run.
Oops. Fixed.
> **7.
`runtime/src/test/scala/org/apache/pekko/grpc/javadsl/GrpcMarshallingSpec.scala:59`
— coverage loss.**
>
> The deleted `"recover a failed unary response stage"` test was the only
unit test asserting that a failed unary `CompletionStage` is turned into a
`grpc-status` response in the Java DSL. The behaviour itself is still correct —
`GrpcExceptionHandler.standard` unwraps `CompletionException` — but it is now
only covered indirectly, via the generated
`unmarshal(...).thenCompose(...).thenApply(...).exceptionally(GrpcExceptionHandler.standard)`
shape. Worth replacing with an equivalent test against that shape.
Will add test coverage for this case.
> **8.
`runtime/src/main/scala/org/apache/pekko/grpc/internal/GrpcProtocolWeb.scala:43`
— parameter typo `frane`.**
Fixed
> Used consistently at line 47, so it compiles and behaves correctly.
Separately: the web variant's `encodeFrame` / `encodeFrameToBytes` still take
`Frame` rather than `OutboundFrame`, unlike the native variant — worth aligning
now that the inbound/outbound split exists.
Fixed
--
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]
