timw commented on PR #862:
URL: https://github.com/apache/pekko-grpc/pull/862#issuecomment-5431549288
> ### Correctness
> **1. `handleUnary` silently drops the caller's `eHandler` when marshalling
fails** —
`runtime/src/main/scala/org/apache/pekko/grpc/scaladsl/GrpcMarshalling.scala`
Good catch. This is why I don't really like default args in general, but
should have caught this.
Fixed with a test.
> **2. `GoogleProtobufSerializer.serialize(t, frame, offset)` ignores
`offset`** —
`runtime/src/main/scala/org/apache/pekko/grpc/javadsl/GoogleProtobufSerializer.scala`
Thanks for spotting.
Fixed with a test.
> ### API / binary compatibility
> **3. Moving the frame-serialization methods onto public
`ProtobufSerializer` is a source and binary break.**
I hadn't realised this was public - originally I didn't like the leaking of
`AbstractGrpcProtocol` mechanics into the serializer, but it makes sense to
retain it as a distinct operation.
Have reinstated this as an optional extension interface with `@ApiMayChange`
as it's a useful opt-in optimisation for an implementation.
> 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.
I'll fix these up once the API surface changes have settled down.
> ### 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.
This is a fair criticism - I was lax in trying to describe essentially what
you have: that these benchmarks are so fine grained that even small tweaks
result in big swings in results that are negligible or compensated for in
overall throughput of higher level benchmarks.
> **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(...)
> ```
Thanks - fixed this and the performance regression is down to 0-3%.
Will redo full benchmarks once some of the other changes have stabilised.
> ### 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.
Fair - have added 'regression' tests for the issues above, but will review
overall test coverage before finalising.
> * **`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.
Fixed
> * **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.
Fixed
> * **`@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.
I did see some small shifts in the benchmark results, but it could be within
the noise - will review and remove them if there's nothing significant that the
JIT doesn't eventually get to.
> * **`BenchRunner`** loses the sorted summary report it used to print —
that's a UX change for anyone running `benchmarks/run` directly. Intentional?
This one was unintentional - I had pruned that code as it was effectively
printing the same results as the default jmh runner output, but I missed the
fact it was sorted. Reinstated with some minor formatting improvements.
> * **`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.
Added comments to this effect.
> * 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.
Fixed as suggested.
> ### 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.
Yeah, I'm not entirely happy with the approach - I like it better than the
existing, which shifts the logic of what consitutes an optimisable protocol
frame out of the GrpcProtocol classes into marshalling code, but there's
probably a middle ground.
I'm pushing a few options as incremental fixup commits:
- A `DeferredDataFrame`, which captures the serialized data output of an
element as a function
- An alternative `DeferredDataFrame`, which captures the element and a
`DefferedDataWriter` instance, with the `ProtobufFrameSerializer` extending that
I'm getting some mixed benchmark results on these, so need to double check
the results a bit.
Of the options, I think I prefer the function based `DeferredDataFrame`.
The other part I didn't really like is introducing an asymmetry `Frame` in
that some of the frames are outbound only now - I've addressed that by
introducing `InboundFrame` and `OutboundFrame` partitions of the `Frame` sealed
trait.
I have the `InboundFrame` concept in my client improvements branch as well,
as it helps clean up the logic there.
Remaining tasks:
- agree on approach for the deferred element write and
`ProtobufFrameSerializer` shape and squash fixup commits
- MiMa updates
- review `@inline` annotations for benefit
- review test coverage
- run full benchmarks with stabilised approach and confirm
- possibly redo PR, or at least rewrite description with clarified
motivation/results
- possibly rebase on main if #863 gets merged first
--
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]
