Optimize flatMapConcat for single element source, #25241#25242
Merged
Optimize flatMapConcat for single element source, #25241#25242
Conversation
akka-ci
added
validating
|
Test FAILed. |
johanandren
reviewed
Jun 18, 2018
Contributor
johanandren
left a comment
johanandren
left a comment
There was a problem hiding this comment.
Very nice numbers. Still a bit sad there isn't a way to make it more general though.
|
|
||
| "work with flatMapConcat optimized GraphStages.SingleSource" in assertAllStagesStopped { | ||
| Source(0 to 3) | ||
| .flatMapConcat(elem ⇒ new GraphStages.SingleSource(elem)) |
Contributor
There was a problem hiding this comment.
Does this mean we should make the constructor of SingleSource a public API or we keep it to ourselves?
akka-ci
added
validating
Contributor
Author
|
I found a way to grab the original
We could get an additional boost to 6743k ops/s by using the I also verified that the extra work doesn't introduce a regression for non single, see BEFORE AFTER |
akka-ci
added
tested
|
Test PASSed. |
akka-ci
added
validating
Contributor
Author
|
Pushed another change to take care of the case when there is no demand b4ea15e |
akka-ci
added
tested
|
Test PASSed. |
johanandren
approved these changes
Jun 20, 2018
| } | ||
|
|
||
| "find Source.single via TraversalBuilder" in assertAllStagesStopped { | ||
| TraversalBuilder.getSingleSource(Source.single("a")).get.elem should ===("a") |
Contributor
There was a problem hiding this comment.
Check also that Source.single.{async/mapMatVal} causes a nested graph and does not match for extra measure? (Pretty sure they do, but just for extra measure.
Contributor
Author
There was a problem hiding this comment.
@johanandren Good point, it was wrong. Matched for those cases also. async is only adding attribute, and mapMaterializedValue is adding Transform traversal. Fixed with additional checks for those in 7ae02d1
patriknw
added
the
2 - pick next
* This first stab is only looking for `SingleSource` and note that `Source.single` is not returning that. Source.single has wrap with fromGraph and then the original `SingleSource` is lost.
patriknw
force-pushed
the
wip-flatMapConcat-patriknw
branch
from
b4ea15e to
7ae02d1
Compare
akka-ci
added
validating
|
Test PASSed. |
johanandren
approved these changes
Jul 11, 2018
| } | ||
|
|
||
| "find Source.single via TraversalBuilder" in assertAllStagesStopped { | ||
| TraversalBuilder.getSingleSource(Source.single("a")).get.elem should ===("a") |
| case _ ⇒ OptionVal.None | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Pyramid of optimization.
raboof
approved these changes
Jul 11, 2018
Contributor
raboof
left a comment
raboof
left a comment
There was a problem hiding this comment.
Sidesteps quite some ceremony in this case 👍
| var q: BufferImpl[SubSinkInlet[T]] = _ | ||
| // To be able to optimize for SingleSource without materializing them the queue may hold either | ||
| // SubSinkInlet[T] or SingleSource | ||
| var queue: BufferImpl[AnyRef] = _ |
| TraversalBuilder.getSingleSource(source.asInstanceOf[Graph[SourceShape[AnyRef], M]]) match { | ||
| case OptionVal.Some(single) ⇒ | ||
| if (isAvailable(out) && queue.isEmpty) { | ||
| push(out, single.elem.asInstanceOf[T]) |
| } | ||
|
|
||
| def removeSource(src: SubSinkInlet[T]): Unit = { | ||
| def removeSource(src: AnyRef): Unit = { |
Contributor
There was a problem hiding this comment.
Maybe explicitly make it private? OK though.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SingleSourceand notethat
Source.singleis not returning that. Source.single haswrap with fromGraph and then the original
SingleSourceislost.
Refs #25241
Benchmark results speak for themselves, 287k vs 6641k elements/s: