You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@pekko.apache.org by "He-Pin (via GitHub)" <gi...@apache.org> on 2023/08/30 14:37:29 UTC
[GitHub] [incubator-pekko-http] He-Pin opened a new pull request, #311: =core Make use of statefulMap instead of statefulMapConcat.
He-Pin opened a new pull request, #311:
URL: https://github.com/apache/incubator-pekko-http/pull/311
Motivation:
Use the `statefulMap` instead when concating is not needed.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] He-Pin commented on a diff in pull request #311: =core Make use of statefulMap instead of statefulMapConcat.
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #311:
URL: https://github.com/apache/incubator-pekko-http/pull/311#discussion_r1311622129
##########
http-core/src/main/scala/org/apache/pekko/http/impl/engine/ws/MessageToFrameRenderer.scala:
##########
@@ -35,20 +34,10 @@ private[http] object MessageToFrameRenderer {
Source.single(FrameEvent.fullFrame(opcode, None, data, fin = true))
def streamedFrames[M](opcode: Opcode, data: Source[ByteString, M]): Source[FrameStart, Any] =
- data.via(StreamUtils.statefulMap(() => {
- var isFirst = true
-
- { data =>
- val frameOpcode =
- if (isFirst) {
- isFirst = false
- opcode
- } else Opcode.Continuation
-
- FrameEvent.fullFrame(frameOpcode, None, data, fin = false)
- }
- })) ++
- Source.single(FrameEvent.emptyLastContinuationFrame)
+ data.statefulMap(() => true)((isFirst, data) => {
+ val frameOpcode = if (isFirst) opcode else Opcode.Continuation
+ (false, FrameEvent.fullFrame(frameOpcode, None, data, fin = false))
Review Comment:
Thanks, there is an benchmark for use the `statefulMap` to implement the `zipWithIndex` which shows no much harm but get better performance.
And it was ` f(i) :: Nil`, plus a `iterator` creation too, I think it's nearly the same.
```
Jmh/run -i 3 -wi 3 -f1 -t1 .*ZipWithIndexBenchmark.*
[info] Benchmark Mode Cnt Score Error Units
[info] ZipWithIndexBenchmark.benchNewZipWithIndex thrpt 3 6501714129.146 锟斤拷 4044285303.660 ops/s
[info] ZipWithIndexBenchmark.benchOldZipWithIndex thrpt 3 6146967182.338 锟斤拷 801852805.114 ops/s
```
That's why I dear to change this.
I can submit a benchmark after worktime to verify this.
##########
http-core/src/main/scala/org/apache/pekko/http/impl/engine/ws/MessageToFrameRenderer.scala:
##########
@@ -35,20 +34,10 @@ private[http] object MessageToFrameRenderer {
Source.single(FrameEvent.fullFrame(opcode, None, data, fin = true))
def streamedFrames[M](opcode: Opcode, data: Source[ByteString, M]): Source[FrameStart, Any] =
- data.via(StreamUtils.statefulMap(() => {
- var isFirst = true
-
- { data =>
- val frameOpcode =
- if (isFirst) {
- isFirst = false
- opcode
- } else Opcode.Continuation
-
- FrameEvent.fullFrame(frameOpcode, None, data, fin = false)
- }
- })) ++
- Source.single(FrameEvent.emptyLastContinuationFrame)
+ data.statefulMap(() => true)((isFirst, data) => {
+ val frameOpcode = if (isFirst) opcode else Opcode.Continuation
+ (false, FrameEvent.fullFrame(frameOpcode, None, data, fin = false))
Review Comment:
Thanks, there is an benchmark for use the `statefulMap` to implement the `zipWithIndex` which shows no much harm but get better performance.
And it was ` f(i) :: Nil`, plus a `iterator` creation too, I think it's nearly the same.
```
Jmh/run -i 3 -wi 3 -f1 -t1 .*ZipWithIndexBenchmark.*
[info] Benchmark Mode Cnt Score Error Units
[info] ZipWithIndexBenchmark.benchNewZipWithIndex thrpt 3 6501714129.146 锟斤拷 4044285303.660 ops/s
[info] ZipWithIndexBenchmark.benchOldZipWithIndex thrpt 3 6146967182.338 锟斤拷 801852805.114 ops/s
```
That's why I dear to change this.
I can submit a benchmark after worktime to verify this.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] He-Pin commented on a diff in pull request #311: =core Make use of statefulMap instead of statefulMapConcat.
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #311:
URL: https://github.com/apache/incubator-pekko-http/pull/311#discussion_r1311622129
##########
http-core/src/main/scala/org/apache/pekko/http/impl/engine/ws/MessageToFrameRenderer.scala:
##########
@@ -35,20 +34,10 @@ private[http] object MessageToFrameRenderer {
Source.single(FrameEvent.fullFrame(opcode, None, data, fin = true))
def streamedFrames[M](opcode: Opcode, data: Source[ByteString, M]): Source[FrameStart, Any] =
- data.via(StreamUtils.statefulMap(() => {
- var isFirst = true
-
- { data =>
- val frameOpcode =
- if (isFirst) {
- isFirst = false
- opcode
- } else Opcode.Continuation
-
- FrameEvent.fullFrame(frameOpcode, None, data, fin = false)
- }
- })) ++
- Source.single(FrameEvent.emptyLastContinuationFrame)
+ data.statefulMap(() => true)((isFirst, data) => {
+ val frameOpcode = if (isFirst) opcode else Opcode.Continuation
+ (false, FrameEvent.fullFrame(frameOpcode, None, data, fin = false))
Review Comment:
Thanks, there is an benchmark for using the `statefulMap` to implement the `zipWithIndex` which shows no much harm but get better performance.
And it was ` f(i) :: Nil`, plus a `iterator` creation too, I think it's nearly the same.
```
Jmh/run -i 3 -wi 3 -f1 -t1 .*ZipWithIndexBenchmark.*
[info] Benchmark Mode Cnt Score Error Units
[info] ZipWithIndexBenchmark.benchNewZipWithIndex thrpt 3 6501714129.146 锟斤拷 4044285303.660 ops/s
[info] ZipWithIndexBenchmark.benchOldZipWithIndex thrpt 3 6146967182.338 锟斤拷 801852805.114 ops/s
```
That's why I dear to change this.
I can submit a benchmark after worktime to verify this.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] He-Pin commented on a diff in pull request #311: =core Make use of statefulMap instead of statefulMapConcat.
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #311:
URL: https://github.com/apache/incubator-pekko-http/pull/311#discussion_r1310903073
##########
http-core/src/main/scala/org/apache/pekko/http/impl/util/StreamUtils.scala:
##########
@@ -260,15 +260,6 @@ private[http] object StreamUtils {
}
}
- /**
- * Similar idea than [[FlowOps.statefulMapConcat]] but for a simple map.
- */
- def statefulMap[T, U](functionConstructor: () => T => U): Flow[T, U, NotUsed] =
- Flow[T].statefulMapConcat { () =>
Review Comment:
The StreamUtil is private[http].
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] =core Make use of statefulMap instead of statefulMapConcat. [incubator-pekko-http]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #311:
URL: https://github.com/apache/incubator-pekko-http/pull/311#discussion_r1435784945
##########
http-bench-jmh/src/main/scala/org/apache/pekko/BenchTestSource.scala:
##########
@@ -0,0 +1,74 @@
+/*
Review Comment:
@He-Pin I will approve this PR if we can work out what license header is needed here.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] =core Make use of statefulMap instead of statefulMapConcat. [incubator-pekko-http]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #311:
URL: https://github.com/apache/incubator-pekko-http/pull/311#issuecomment-1868460672
@He-Pin can you rebase this PR - it's quite old
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] pjfanning commented on a diff in pull request #311: =core Make use of statefulMap instead of statefulMapConcat.
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #311:
URL: https://github.com/apache/incubator-pekko-http/pull/311#discussion_r1310741055
##########
http-core/src/main/scala/org/apache/pekko/http/impl/util/StreamUtils.scala:
##########
@@ -260,15 +260,6 @@ private[http] object StreamUtils {
}
}
- /**
- * Similar idea than [[FlowOps.statefulMapConcat]] but for a simple map.
- */
- def statefulMap[T, U](functionConstructor: () => T => U): Flow[T, U, NotUsed] =
- Flow[T].statefulMapConcat { () =>
Review Comment:
Can you deprecate this? We can't remove public methods without deprecating them 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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] He-Pin commented on a diff in pull request #311: =core Make use of statefulMap instead of statefulMapConcat.
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #311:
URL: https://github.com/apache/incubator-pekko-http/pull/311#discussion_r1310903073
##########
http-core/src/main/scala/org/apache/pekko/http/impl/util/StreamUtils.scala:
##########
@@ -260,15 +260,6 @@ private[http] object StreamUtils {
}
}
- /**
- * Similar idea than [[FlowOps.statefulMapConcat]] but for a simple map.
- */
- def statefulMap[T, U](functionConstructor: () => T => U): Flow[T, U, NotUsed] =
- Flow[T].statefulMapConcat { () =>
Review Comment:
The StreamUtil is private[http]. And. Marked as internalApi
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] He-Pin commented on a diff in pull request #311: =core Make use of statefulMap instead of statefulMapConcat.
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #311:
URL: https://github.com/apache/incubator-pekko-http/pull/311#discussion_r1311622129
##########
http-core/src/main/scala/org/apache/pekko/http/impl/engine/ws/MessageToFrameRenderer.scala:
##########
@@ -35,20 +34,10 @@ private[http] object MessageToFrameRenderer {
Source.single(FrameEvent.fullFrame(opcode, None, data, fin = true))
def streamedFrames[M](opcode: Opcode, data: Source[ByteString, M]): Source[FrameStart, Any] =
- data.via(StreamUtils.statefulMap(() => {
- var isFirst = true
-
- { data =>
- val frameOpcode =
- if (isFirst) {
- isFirst = false
- opcode
- } else Opcode.Continuation
-
- FrameEvent.fullFrame(frameOpcode, None, data, fin = false)
- }
- })) ++
- Source.single(FrameEvent.emptyLastContinuationFrame)
+ data.statefulMap(() => true)((isFirst, data) => {
+ val frameOpcode = if (isFirst) opcode else Opcode.Continuation
+ (false, FrameEvent.fullFrame(frameOpcode, None, data, fin = false))
Review Comment:
Thanks, there is an benchmark for using the `statefulMap` to implement the `zipWithIndex` which shows no much harm but get better performance.
And it was ` f(i) :: Nil`, plus a `iterator` creation too, I think it's nearly the same.
```
Jmh/run -i 3 -wi 3 -f1 -t1 .*ZipWithIndexBenchmark.*
[info] Benchmark Mode Cnt Score Error Units
[info] ZipWithIndexBenchmark.benchNewZipWithIndex thrpt 3 6501714129.146 锟斤拷 4044285303.660 ops/s
[info] ZipWithIndexBenchmark.benchOldZipWithIndex thrpt 3 6146967182.338 锟斤拷 801852805.114 ops/s
```
That's why I dear to change this.
I can submit a benchmark after worktime to verify this，And I think the JVM should be smart enough to do Threadlocal stack allocation for this.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] =core Make use of statefulMap instead of statefulMapConcat. [incubator-pekko-http]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #311:
URL: https://github.com/apache/incubator-pekko-http/pull/311#discussion_r1435785019
##########
http-bench-jmh/src/main/scala/org/apache/pekko/BenchTestSource.scala:
##########
@@ -0,0 +1,74 @@
+/*
Review Comment:
Yes, I copied it from pekko, where it has a lightbend header.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] =core Make use of statefulMap instead of statefulMapConcat. [incubator-pekko-http]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #311:
URL: https://github.com/apache/incubator-pekko-http/pull/311#discussion_r1435783053
##########
http-bench-jmh/src/main/scala/org/apache/pekko/BenchTestSource.scala:
##########
@@ -0,0 +1,74 @@
+/*
Review Comment:
should this file have a Lightbend header?
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] He-Pin commented on a diff in pull request #311: =core Make use of statefulMap instead of statefulMapConcat.
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #311:
URL: https://github.com/apache/incubator-pekko-http/pull/311#discussion_r1311781340
##########
http-core/src/main/scala/org/apache/pekko/http/impl/engine/ws/MessageToFrameRenderer.scala:
##########
@@ -35,20 +34,10 @@ private[http] object MessageToFrameRenderer {
Source.single(FrameEvent.fullFrame(opcode, None, data, fin = true))
def streamedFrames[M](opcode: Opcode, data: Source[ByteString, M]): Source[FrameStart, Any] =
- data.via(StreamUtils.statefulMap(() => {
- var isFirst = true
-
- { data =>
- val frameOpcode =
- if (isFirst) {
- isFirst = false
- opcode
- } else Opcode.Continuation
-
- FrameEvent.fullFrame(frameOpcode, None, data, fin = false)
- }
- })) ++
- Source.single(FrameEvent.emptyLastContinuationFrame)
+ data.statefulMap(() => true)((isFirst, data) => {
+ val frameOpcode = if (isFirst) opcode else Opcode.Continuation
+ (false, FrameEvent.fullFrame(frameOpcode, None, data, fin = false))
Review Comment:
```
[info] Benchmark Mode Cnt Score Error Units
[info] MessageToFrameRendererBenchmark.benchNewStreamedFrames thrpt 10 25333318.747 锟斤拷 1293108.754 ops/s
[info] MessageToFrameRendererBenchmark.benchOldStreamedFrames thrpt 10 20352150.287 锟斤拷 1563364.567 ops/s
```
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] pjfanning commented on a diff in pull request #311: =core Make use of statefulMap instead of statefulMapConcat.
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #311:
URL: https://github.com/apache/incubator-pekko-http/pull/311#discussion_r1311573115
##########
http-core/src/main/scala/org/apache/pekko/http/impl/util/StreamUtils.scala:
##########
@@ -260,15 +260,6 @@ private[http] object StreamUtils {
}
}
- /**
- * Similar idea than [[FlowOps.statefulMapConcat]] but for a simple map.
- */
- def statefulMap[T, U](functionConstructor: () => T => U): Flow[T, U, NotUsed] =
- Flow[T].statefulMapConcat { () =>
Review Comment:
I'm not against taking this out then but can you locally run `sbt mimaReportBinaryIssues` on this branch? I think there will be an issue and that you will need to add a mima-filters file.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] He-Pin commented on a diff in pull request #311: =core Make use of statefulMap instead of statefulMapConcat.
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #311:
URL: https://github.com/apache/incubator-pekko-http/pull/311#discussion_r1311610822
##########
http-core/src/main/scala/org/apache/pekko/http/impl/util/StreamUtils.scala:
##########
@@ -260,15 +260,6 @@ private[http] object StreamUtils {
}
}
- /**
- * Similar idea than [[FlowOps.statefulMapConcat]] but for a simple map.
- */
- def statefulMap[T, U](functionConstructor: () => T => U): Flow[T, U, NotUsed] =
- Flow[T].statefulMapConcat { () =>
Review Comment:
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] =core Make use of statefulMap instead of statefulMapConcat. [incubator-pekko-http]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #311:
URL: https://github.com/apache/incubator-pekko-http/pull/311#discussion_r1435784844
##########
http-bench-jmh/src/main/scala/org/apache/pekko/BenchTestSource.scala:
##########
@@ -0,0 +1,74 @@
+/*
Review Comment:
Let me check
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] =core Make use of statefulMap instead of statefulMapConcat. [incubator-pekko-http]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on PR #311:
URL: https://github.com/apache/incubator-pekko-http/pull/311#issuecomment-1868447366
@jrudolph @pjfanning @mdedetrich ping~
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] =core Make use of statefulMap instead of statefulMapConcat. [incubator-pekko-http]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin merged PR #311:
URL: https://github.com/apache/incubator-pekko-http/pull/311
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] He-Pin commented on a diff in pull request #311: =core Make use of statefulMap instead of statefulMapConcat.
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #311:
URL: https://github.com/apache/incubator-pekko-http/pull/311#discussion_r1311781340
##########
http-core/src/main/scala/org/apache/pekko/http/impl/engine/ws/MessageToFrameRenderer.scala:
##########
@@ -35,20 +34,10 @@ private[http] object MessageToFrameRenderer {
Source.single(FrameEvent.fullFrame(opcode, None, data, fin = true))
def streamedFrames[M](opcode: Opcode, data: Source[ByteString, M]): Source[FrameStart, Any] =
- data.via(StreamUtils.statefulMap(() => {
- var isFirst = true
-
- { data =>
- val frameOpcode =
- if (isFirst) {
- isFirst = false
- opcode
- } else Opcode.Continuation
-
- FrameEvent.fullFrame(frameOpcode, None, data, fin = false)
- }
- })) ++
- Source.single(FrameEvent.emptyLastContinuationFrame)
+ data.statefulMap(() => true)((isFirst, data) => {
+ val frameOpcode = if (isFirst) opcode else Opcode.Continuation
+ (false, FrameEvent.fullFrame(frameOpcode, None, data, fin = false))
Review Comment:
```
[info] Benchmark Mode Cnt Score Error Units
[info] MessageToFrameRendererBenchmark.benchNewStreamedFrames thrpt 10 15148931.446 锟斤拷 99406.646 ops/s
[info] MessageToFrameRendererBenchmark.benchOldStreamedFrames thrpt 10 13296638.654 锟斤拷 553890.759 ops/s
```
##########
http-core/src/main/scala/org/apache/pekko/http/impl/engine/ws/MessageToFrameRenderer.scala:
##########
@@ -35,20 +34,10 @@ private[http] object MessageToFrameRenderer {
Source.single(FrameEvent.fullFrame(opcode, None, data, fin = true))
def streamedFrames[M](opcode: Opcode, data: Source[ByteString, M]): Source[FrameStart, Any] =
- data.via(StreamUtils.statefulMap(() => {
- var isFirst = true
-
- { data =>
- val frameOpcode =
- if (isFirst) {
- isFirst = false
- opcode
- } else Opcode.Continuation
-
- FrameEvent.fullFrame(frameOpcode, None, data, fin = false)
- }
- })) ++
- Source.single(FrameEvent.emptyLastContinuationFrame)
+ data.statefulMap(() => true)((isFirst, data) => {
+ val frameOpcode = if (isFirst) opcode else Opcode.Continuation
+ (false, FrameEvent.fullFrame(frameOpcode, None, data, fin = false))
Review Comment:
```
[info] Benchmark Mode Cnt Score Error Units
[info] MessageToFrameRendererBenchmark.benchNewStreamedFrames thrpt 10 15148931.446 锟斤拷 99406.646 ops/s
[info] MessageToFrameRendererBenchmark.benchOldStreamedFrames thrpt 10 13296638.654 锟斤拷 553890.759 ops/s
```
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] He-Pin commented on a diff in pull request #311: =core Make use of statefulMap instead of statefulMapConcat.
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #311:
URL: https://github.com/apache/incubator-pekko-http/pull/311#discussion_r1310903073
##########
http-core/src/main/scala/org/apache/pekko/http/impl/util/StreamUtils.scala:
##########
@@ -260,15 +260,6 @@ private[http] object StreamUtils {
}
}
- /**
- * Similar idea than [[FlowOps.statefulMapConcat]] but for a simple map.
- */
- def statefulMap[T, U](functionConstructor: () => T => U): Flow[T, U, NotUsed] =
- Flow[T].statefulMapConcat { () =>
Review Comment:
The StreamUtil is private[http]. And. Marked as internalApi, so this change should be fine
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] jrudolph commented on a diff in pull request #311: =core Make use of statefulMap instead of statefulMapConcat.
Posted by "jrudolph (via GitHub)" <gi...@apache.org>.
jrudolph commented on code in PR #311:
URL: https://github.com/apache/incubator-pekko-http/pull/311#discussion_r1311584528
##########
http-core/src/main/scala/org/apache/pekko/http/impl/engine/ws/MessageToFrameRenderer.scala:
##########
@@ -35,20 +34,10 @@ private[http] object MessageToFrameRenderer {
Source.single(FrameEvent.fullFrame(opcode, None, data, fin = true))
def streamedFrames[M](opcode: Opcode, data: Source[ByteString, M]): Source[FrameStart, Any] =
- data.via(StreamUtils.statefulMap(() => {
- var isFirst = true
-
- { data =>
- val frameOpcode =
- if (isFirst) {
- isFirst = false
- opcode
- } else Opcode.Continuation
-
- FrameEvent.fullFrame(frameOpcode, None, data, fin = false)
- }
- })) ++
- Source.single(FrameEvent.emptyLastContinuationFrame)
+ data.statefulMap(() => true)((isFirst, data) => {
+ val frameOpcode = if (isFirst) opcode else Opcode.Continuation
+ (false, FrameEvent.fullFrame(frameOpcode, None, data, fin = false))
Review Comment:
How much more boxing and allocations will that produce?
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org