Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Streamed response processing performance improvements #2645

Merged

Conversation

@jrudolph
Copy link
Member

commented Aug 14, 2019

Culprits were Future.failed and Source.concat. This improved speed for streamed responses from ~20x slower to only ~10x slower than strict responses (which is still a lot).

@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Aug 14, 2019

Test FAILed.

@raboof

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

Jenkins:

java.lang.AssertionError: expected OnNext, found OnError(java.lang.IllegalArgumentException: Cannot push port (HttpResponseRenderer.out(1347198232)) twice, or before it being pulled
	at akka.stream.stage.GraphStageLogic.push(GraphStage.scala:623)
	at akka.http.impl.engine.rendering.HttpResponseRendererFactory$HttpResponseRenderer$$anon$1$$anon$2.onPush(HttpResponseRendererFactory.scala:93)
@jrudolph jrudolph force-pushed the jrudolph:jr/streamed-response-processing-perf-improvements branch from 0cabc8f to 5ce4b93 Sep 10, 2019
@jrudolph

This comment has been minimized.

Copy link
Member Author

commented Sep 10, 2019

Jenkins:

java.lang.AssertionError: expected OnNext, found OnError(java.lang.IllegalArgumentException: Cannot push port (HttpResponseRenderer.out(1347198232)) twice, or before it being pulled
	at akka.stream.stage.GraphStageLogic.push(GraphStage.scala:623)
	at akka.http.impl.engine.rendering.HttpResponseRendererFactory$HttpResponseRenderer$$anon$1$$anon$2.onPush(HttpResponseRendererFactory.scala:93)

That was interesting. The problem here was that headers are sent out before materialization but that didn't take into account that I might want to also handle errors during materialization with an error response. I now moved sending out headers into the substream.

@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Sep 10, 2019

Test PASSed.

@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Sep 10, 2019

Test PASSed.

Copy link
Member

left a comment

Changes look good, perhaps remove renderByteStrings

case Failure(ex) =>
log.error(ex, s"Response stream for [${requestStart.debugString}] failed with '${ex.getMessage}'. Aborting connection.")
case _ => // ignore
}(ExecutionContexts.sameThreadExecutionContext)

This comment has been minimized.

Copy link
@raboof
@@ -70,7 +70,7 @@ private[rendering] object RenderSupport {
skipEntity: Boolean = false): Source[ByteString, Any] = {
val messageStart = Source.single(header)
val messageBytes =
if (!skipEntity) (messageStart ++ entityBytes).mapMaterializedValue(_ => ())
if (!skipEntity) messageStart ++ entityBytes

This comment has been minimized.

Copy link
@raboof
@@ -240,8 +249,16 @@ private[http] class HttpResponseRendererFactory(
def renderContentLengthHeader(contentLength: Long) =
if (status.allowsEntity) r ~~ `Content-Length` ~~ contentLength ~~ CrLf else r

def byteStrings(entityBytes: => Source[ByteString, Any]): Source[ResponseRenderingOutput, Any] =
renderByteStrings(r.asByteString, entityBytes, skipEntity = noEntity).map(ResponseRenderingOutput.HttpData(_))

This comment has been minimized.

Copy link
@raboof

raboof Sep 13, 2019

Member

Ah, so the concat was in here, and is replaced with simply separate 'push' calls here. Makes sense.

It seems renderByteStrings is now unused entirely, so perhaps we should remove it?

@raboof raboof force-pushed the jrudolph:jr/streamed-response-processing-perf-improvements branch from c969b7f to 02cc72b Sep 16, 2019
@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Sep 16, 2019

Test FAILed.

!!! Couldn't read commit file !!!

jrudolph added 5 commits Aug 14, 2019
…slow

When the future is successful it will always create a `NoSuchElementException`
including stack trace.
It turned out that aside from materialization which is still somewhat slow
at least some bits of slowness were transferred from materialization to building
stream graphs. The culprit in this case is `Source.concat`, which is really
slow.
fix
@raboof raboof force-pushed the jrudolph:jr/streamed-response-processing-perf-improvements branch from 02cc72b to 9dcb7ea Sep 16, 2019
@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Sep 16, 2019

Test PASSed.

Pull request validation report

@raboof
raboof approved these changes Sep 17, 2019
@raboof raboof merged commit f05f026 into akka:master Sep 17, 2019
3 of 4 checks passed
3 of 4 checks passed
Jenkins PR Auto-Formatter Failed
Details
Jenkins PR Validation Test PASSed. 4172 tests run, 1074 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
typesafe-cla-validator All users have signed the CLA
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.