-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Update Streams to 2.4.3 #2292
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
Update Streams to 2.4.3 #2292
Conversation
This performance test is failed - Akka.Streams.Tests.Performance.GraphBuilderBenchmark+Graph_with_1000_imported_flows Also I've found a lot of similar exceptions in performance tests, like this
|
@Silv3rcircl3 you should check this test Xunit can't execute it and just fails. I've checked it 3 times |
@@ -88,14 +92,16 @@ public void A_Graph_with_materialized_value_must_expose_the_materialized_value_a | |||
return new SourceShape<Task<int>>(b.MaterializedValue); | |||
})); | |||
|
|||
[Fact] | |||
public void A_Graph_with_materialized_value_must_allow_exposing_the_materialized_value_as_port() | |||
[InlineData(true, false)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably you should use
[InlineData(true)]
[InlineData(false)]
04ef79f
to
9807270
Compare
@alexvaluyskiy addressed your remarks |
LGTM |
just needs a rebase |
9807270
to
5120498
Compare
@Aaronontheweb rebased but @Horusiath wanted to take a look too so he should merge it once he has reviewed it. |
@@ -82,7 +81,7 @@ public override IModule WithAttributes(Attributes attributes) | |||
protected override SinkModule<NotUsed, Task<Attributes>> NewInstance(SinkShape<NotUsed> shape) | |||
=> new AttributesSink(Attributes, shape); | |||
|
|||
public override ISubscriber<NotUsed> Create(MaterializationContext context, out Task<Attributes> materializer) | |||
public override object Create(MaterializationContext context, out Task<Attributes> materializer) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why object instead of ISubscriber
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because that can now also return a Publisher 😄
See https://github.com/akkadotnet/akka.net/pull/2292/files#diff-a4bb8f441c4b4960d165988a8f8473c4R113
and
https://github.com/akkadotnet/akka.net/pull/2292/files#diff-a4bb8f441c4b4960d165988a8f8473c4R52
This is enormous amount of work, but guys - that performance drop seriously worries me (and no, setting perf threshold to be more forgiving is not a way to go). From what I know, performance in JVM never drops in higher versions, and I think we should not let this happen on our side. This is strange, as i.e. |
The perf specs passed yesterday and today failed with this ~50% perf drop without any code changes. So I guess it has something to do with the build server. |
For example the Graph_with_1000_junction failed in the last run but passed the last 3 runs before |
closes #2263