apache / pekko-http

The Streaming-first HTTP server/module of Apache Pekko
https://pekko.apache.org/
Apache License 2.0
148 stars 34 forks source link

Reverted: use optimised ByteString.asInputStream #539

Closed pjfanning closed 2 months ago

pjfanning commented 3 months ago
pjfanning commented 2 months ago

I benchmarked this using https://github.com/pjfanning/pekko-test/blob/main/bench/src/main/scala/org/example/pekko/ByteStringBench.scala

The new code (in this PR) was faster for byte strings that are made up of multiple byte strings but slower for simple byte strings.

[info] Benchmark                                 Mode  Cnt       Score        Error  Units
[info] ByteStringBench.newGetInputStream        thrpt    3     752.065 ±    787.928  ops/s
[info] ByteStringBench.newGetInputStreamSimple  thrpt    3  447966.746 ± 273762.965  ops/s
[info] ByteStringBench.oldGetInputStream        thrpt    3     535.979 ±   1080.417  ops/s
[info] ByteStringBench.oldGetInputStreamSimple  thrpt    3  486935.775 ± 222571.828  ops/s

While the speedup for complex byte strings is good and the slowdown for simple byte strings is proportionally small - it might not be worth the hit.

I presume that the MethodHandle adds an overhead.

pjfanning commented 2 months ago

Now have after https://github.com/pjfanning/pekko-test/commit/a79e14f66cedce61d07275aa5639f13a9668af60

[info] Benchmark                                 Mode  Cnt       Score        Error  Units
[info] ByteStringBench.newGetInputStream        thrpt    3     816.413 ±    114.507  ops/s
[info] ByteStringBench.newGetInputStreamSimple  thrpt    3  492976.928 ± 145281.777  ops/s
[info] ByteStringBench.oldGetInputStream        thrpt    3     588.401 ±    117.303  ops/s
[info] ByteStringBench.oldGetInputStreamSimple  thrpt    3  490941.644 ± 119759.598  ops/s
laglangyue commented 2 months ago

LGTM