use ByteStringInputStream #309
Conversation
runtime/src/main/scala/org/apache/pekko/grpc/internal/ByteStringInputStream.scala
Outdated
Show resolved
Hide resolved
Co-Authored-By: João Ferreira <[email protected]>
| def apply(bs: ByteString): InputStream = bs match { | ||
| case cs: ByteString1C => | ||
| getInputStreamUnsafe(cs) | ||
| case _ => | ||
| if (byteStringInputStreamMethodTypeOpt.isDefined) { | ||
| byteStringInputStreamMethodTypeOpt.get.invoke(bs).asInstanceOf[InputStream] | ||
| } else { | ||
| legacyConvert(bs) | ||
| } | ||
| } |
There was a problem hiding this comment.
and what about:
| def apply(bs: ByteString): InputStream = bs match { | |
| case cs: ByteString1C => | |
| getInputStreamUnsafe(cs) | |
| case _ => | |
| if (byteStringInputStreamMethodTypeOpt.isDefined) { | |
| byteStringInputStreamMethodTypeOpt.get.invoke(bs).asInstanceOf[InputStream] | |
| } else { | |
| legacyConvert(bs) | |
| } | |
| } | |
| def apply(bs: ByteString): InputStream = { | |
| if (byteStringInputStreamMethodTypeOpt.isDefined) { | |
| byteStringInputStreamMethodTypeOpt.get.invoke(bs).asInstanceOf[InputStream] | |
| } else { | |
| legacyConvert(bs) | |
| } | |
| } |
There was a problem hiding this comment.
another improvement could be the use of null instead of None in byteStringInputStreamMethodTypeOpt
There was a problem hiding this comment.
I'll check it again but when I was benchmarking the changes before - having the ByteString1C case seemed to help performance. I can't track down the benchmarks that I used, I may need to write them again.
There was a problem hiding this comment.
actually - the benchmark code that I used a few weeks ago is at https://github.com/pjfanning/pekko-test/
There was a problem hiding this comment.
With your optimised version of legacyConvert, we may not need the MethodHandle. That code is basically what adInputStream does anyway.
There was a problem hiding this comment.
I checked in the simpler version. It seems good enough and Pekko 1.0 users benefit with this version.
Co-Authored-By: João Ferreira <[email protected]>
|
@He-Pin I reworked the code. Are you still happy with this change? |
|
@jtjeferreira It isn't strictly necessary but could you consider supplying an iCLA (if you haven't done so already)? |
|
lgtm |
follow up to #308