Refactor Props #5416

Zetanova · 2021-12-04T17:13:21Z

refactor of class Props to reduce view allocations, seal the class,
and adding the possibility to use the same IIndirectActorProducer for multiple Props instances.

There is new an new obsolete IIndirectActorProducerWithActorType for legacy Akka.DI.Core,
but I don't know the legacy DI well to know if there will be some binary combability issues.

Improvements for each props instance:
1x less object instance with min. 1 reference field (the producer instance)
1x less reference field (outtype field)
View less vcalls

Zetanova · 2021-12-04T17:20:14Z

src/contrib/dependencyinjection/Akka.DI.Core/DIExt.cs

@@ -38,7 +38,7 @@ public void Initialize(IDependencyResolver dependencyResolver)
 /// <returns>A <see cref="Akka.Actor.Props"/> configuration object for the given actor type.</returns>
 public Props Props(Type actorType)
 {
- return new Props(typeof(DIActorProducer), new object[] { dependencyResolver, actorType });


If this was the only place where the old DIActorProducer got with new object[] { dependencyResolver, actorType } created, then we could remove the obsolete Props.CreateProducer method.

That's true. Can mark that API for deletion in 1.5

how do we mark deletion in version?
simply put some text in the obsolete attribute message?

Correct - just mention that in the ObsoleteAttribute's text

Zetanova · 2021-12-04T17:28:39Z

src/core/Akka/Actor/Props.cs

+ if (producer is IIndirectActorProducerWithActorType p)
+ return (producer, p.ActorType, NoArgs);
+
+ //maybe throw new ArgumentException($"Unsupported legacy actor producer [{type.FullName}]", nameof(type));


here I am not sure, maybe to throw is better

If it's actually unsupported now, we should throw here.

Zetanova · 2021-12-04T17:30:54Z

src/core/Akka/Actor/Props.cs

@@ -689,92 +669,20 @@ public FactoryConsumer(Func<TActor> factory)
 _factory = factory;
 }

- public ActorBase Produce()
+ public ActorBase Produce(Props props)
 {
 return _factory.Invoke();


maybe insert here a safety type check with props.Type ?

To do what with it exactly? I don't recall what the FactoryConsumer does anymore

FactoryConsumer was not used, I reactivated it for the Producer by Custom-Functon

Zetanova · 2021-12-05T01:08:54Z

src/core/Akka/Actor/Props.cs

- return CompareDeploy(other) && CompareSupervisorStrategy(other) && CompareArguments(other) &&
- CompareInputType(other);
+ return Type == other.Type
+ && _producer.GetType() == other._producer.GetType()


I added here a simple producer type compare, there was no comparison on the producer before

One other idea would be to let test if the producer implements IEqualityComparer<Akka.Props>
or maybe add a field for it into the producer interface.

Zetanova · 2021-12-05T14:00:48Z

All tests running successfully on my local machine.
The probe.ExpectMsg(...) got somehow very racy, didn't make any changes related to this method.

Is the build server just over committed or throttled and just slow?

Zetanova · 2021-12-06T07:05:04Z

@Aaronontheweb Do we have any unit tests for the TestKit methods itself, that are checking the thread-id?
the test method like probe.ExpectMsg(...) should not run on a dispachter thread.

I do not have currently an other idea.
On view racy tests the first probe.ExpectMsg(...) was successful and the other inside a Within(...) block just failed.
The probe.ExpectMsg(...) method dequeues in blocking from a BlockingCollection with timeout and cancellation set.

Aaronontheweb · 2021-12-06T15:56:14Z

@Zetanova the test suite has become much more flakey since the .NET 6 upgrade due to the new "deadlock detection" algos that the managed thread pool uses. We're going to need to refactor all of it to be async probably, which we'd already started gradually doing.

Zetanova · 2021-12-06T16:05:40Z

What exactly happens with the new "deadlock detection" ?

The message is not pushed into the Queue of TestProp.
This can only mean 3 things:

the adding thread of to the queue and the waiting thread are the same.
the actor-system had internal errors and try-catch-ignore them
the actor-system add log-errors over AkkaLogger that itself uses a Mailbox.
The effect is that Log messages getting ignored at startup or do not write out after a failed test

Aaronontheweb · 2021-12-06T16:11:49Z

What exactly happens with the new "deadlock detection" ?

I'm not entirely sure, but the behavior of the ThreadPool now means that new threads will be spawned when a thread is blocking for longer than it "should" based on some heuristics. We need to basically start awaiting on things inside our test suite to avoid triggering this, rather than blocking.

Zetanova · 2021-12-06T16:30:51Z

Don't think that's, even with a new thread the message should be in the BlockingCollection in under 300ms.

Would be most likely better to find and fix first the issue in the TestKit and then update the test methods to async
I found one logically racy test ShouldFailIfMoreExceptionsThenSpecifiedAreLogged and fixed it in this PR.

For probe.ExpectMsg(...) and Within(...) i will look if there are already tests written for thread-Id-checks
and if not add one or two for it

Zetanova · 2021-12-07T11:33:37Z

Some tests failed only in "debug" test run, but not with a simple "run" test
Both in debug builds

I found In the CircuitBreaker a sync method that waited only 50ms for execution.
The task just didn't get activated/run in the period by the threadpool/task-scheduler
I could refactor the method to fix it.

But the underlining problem is most likely that scheduled task just do not activated in time,
specially with a very low timeout (under 50ms)

Zetanova · 2021-12-07T12:07:14Z

This failed test in streams is strange
The actor system begins to shutdown near instantly at test-start
even before the timeout in the test itself

This indicates some startup-error and lost-log messages,
like mention in a comments above

Zetanova · 2021-12-07T15:07:42Z

@Aaronontheweb two failing tests are left, most likely it's just the racy akka-system startup or?
The test do not fail locally in debug-test-run and normal test-run

Zetanova · 2021-12-08T09:45:43Z

@Aaronontheweb I added now a hack into the TestPrope.Create method
fcf18c2

This produces now errors local and a lot on the build-server, strange only in Akka.Cluster.Scharding and Akka.Persistence
and all other failed test are gone.

Aaronontheweb · 2021-12-08T14:43:55Z

You have a lot of test failures now (77) - looks like one of your changes broke something in the logging system that the TestKit uses.

Zetanova · 2021-12-08T17:53:54Z

I put a hack inside the commit to instantly await a log message and it is failing now

Need to analyze why it is not support one some tests

Zetanova · 2021-12-08T18:55:28Z

I removed now the testprope startup await hack,
will try to find the racy component locally and create a new PR for it,
but it is not from this PR

@Aaronontheweb if its ok for can be merged

Aaronontheweb · 2021-12-13T13:40:56Z

Finally looking at this now

Aaronontheweb

Mostly looks good, but have some changes that either need to be explained in more detail or removed

Aaronontheweb · 2021-12-13T13:49:23Z

src/contrib/dependencyinjection/Akka.DI.Core/DIExt.cs

@@ -38,7 +38,7 @@ public void Initialize(IDependencyResolver dependencyResolver)
 /// <returns>A <see cref="Akka.Actor.Props"/> configuration object for the given actor type.</returns>
 public Props Props(Type actorType)
 {
- return new Props(typeof(DIActorProducer), new object[] { dependencyResolver, actorType });


That's true. Can mark that API for deletion in 1.5

src/contrib/dependencyinjection/Akka.DependencyInjection/ServiceProvider.cs

src/core/Akka.API.Tests/CoreAPISpec.ApproveCore.approved.txt

Aaronontheweb · 2021-12-13T13:51:58Z

src/core/Akka.API.Tests/CoreAPISpec.ApproveCore.approved.txt

@@ -1072,10 +1072,14 @@ namespace Akka.Actor
 }
 public interface IIndirectActorProducer
 {
- System.Type ActorType { get; }
- Akka.Actor.ActorBase Produce();


I need to check to see if there are any instances of plugins that call this - I don't think so but I want to double check.

I think we're good

src/core/Akka.API.Tests/CoreAPISpec.ApproveCore.approved.txt

src/core/Akka/Event/Logging.cs

src/core/Akka/Actor/Props.cs

Aaronontheweb · 2021-12-13T14:33:53Z

src/core/Akka/Actor/Props.cs

@@ -689,92 +669,20 @@ public FactoryConsumer(Func<TActor> factory)
 _factory = factory;
 }

- public ActorBase Produce()
+ public ActorBase Produce(Props props)
 {
 return _factory.Invoke();


To do what with it exactly? I don't recall what the FactoryConsumer does anymore

src/core/Akka/Actor/Props.cs

Aaronontheweb · 2021-12-13T14:36:34Z

src/core/Akka/Actor/Props.cs

@@ -32,12 +30,12 @@ namespace Akka.Actor
 /// </code>
 /// </example>
 /// </summary>
- public class Props : IEquatable<Props>, ISurrogated
+ public sealed class Props : IEquatable<Props>, ISurrogated


I'm not sure how I feel about this, in terms of keeping this class open for extension. I checked the Phobos source to see if we subclass Props anywhere there and we do not (we do subclass Deploy, one of the sub-components of Props) so if that doesn't need to extend Props I can't imagine many other plugins that do...

I don't think that it has subclasses anywhere.
It's a miss design to use subclasses as decorators and/or to change the behavior.

Props is a data/payload structure and the IIndirectActorProducer should contain the behavior.

Aaronontheweb

LGTM - we're doing the 1.4.29 release today and I don't want to incorporate this change right away without having some further testing on subsequent PRs (I did the same thing with my PipeTo changes between 1.4.28 and 1.4.29), but this will make it into 1.4.30

to11mtm · 2021-12-10T00:17:45Z

src/contrib/dependencyinjection/Akka.DI.Core/DIActorProducer.cs

 {
- return actorFactory();
+ if (props.Type != _actorType)


Do we need to be more careful about inheritance checks here?

This is part of the deprecated DI library so I'm not as worried about it - but did we end up changing something here that we shouldn't have?

I insert here only a safty check the props.Type and _actorType should have the same type,
because it's only copy

to11mtm · 2021-12-10T00:33:51Z

src/core/Akka.TestKit/ActorCellKeepingSynchronizationContext.cs

 {
+ var t = ((SendOrPostCallback, object, ActorCellKeepingSynchronizationContext, ActorCell))s;


I'm torn on this. I feel like once we multitarget netcore, this can be made better (because then, I think the boxing/unboxing can be elided here). But for now we are paying box/unbox penalty.

But as it stands, is it worth deconstructing the tuple immediately to help with reads?

This only affects the TestKit so I think we should be ok

I don't know what's faster here ValueTuple or System.Tuple, wanted only remove the scope.

It does not matter here, but when a scope is used with ThreadPool,
the context will not be GC until the action runs, it can lead to a self-reference in some scenarios.

to11mtm · 2021-12-14T02:31:52Z

src/core/Akka/Actor/Props.cs

 var args = newExpression.GetArguments();
+ args = args.Length > 0 ? args : NoArgs;

- return new Props(new ActivatorProducer(typeof(TActor), args), DefaultDeploy, args){ SupervisorStrategy = supervisorStrategy };
+ return new Props(ActivatorProducer.Instance, DefaultDeploy, typeof(TActor), args) { SupervisorStrategy = supervisorStrategy };


If we're already getting the args, would there be any benefit in also sniffing out the Ctor in .GetArguments()? (Can we even do that in a way that works OK with expectations of Props?)

That method was originally CopyPasta'd from OddJob, where we weren't newing up objects but rather were getting arguments for method calls. If you can actually hang on to what constructor is called (would be easy) there are opportunities to elide penalties involved with Activator.CreateInstance

Can you file an issue for this so we remember to do this as part of 1.5?

@to11mtm We could move the logic into a custom-actor-producer and use the instance for mass creation of the same actor type with the same or different props-instances,
everything else will always require runtime binding or dynamic-compile and use it only once

to11mtm · 2021-12-14T02:37:26Z

src/core/Akka/Actor/Props.cs

 var args = newExpression.GetArguments();
+ args = args.Length > 0 ? args : NoArgs;


Consider, if you don't do the deed commented below about pulling ConstructorInfo:

var args = newExpression.Arguments > 0 ? newExpression.GetArguments() : NoArgs;

In the call chain for .GetArguments() it looks like we might allocate an empty array if a 0-arg ctor is passed in. The suggestion will elide that allocation.

That's a good idea

to11mtm · 2021-12-14T02:44:18Z

src/core/Akka/Actor/Props.cs

 {
-
+ Deploy = deploy;
+ // have to preserve the "CreateProducer" call here to preserve backwards compat with Akka.DI.Core


Thank you. :)

per akkadotnet#5416 (comment)

per #5416 (comment)

per akkadotnet#5416 (comment)

) * remove the forced waiting on the underlying transport to start up * racy spec fix: `BackoffOnRestartSupervisor_must_respect_maxNrOfRetries_property_of_OneForOneStrategy` (#5442) issue was that using `AutoReset` could cause a race where if the scheduler could reset the count in-flight and throw off the estimates used by the tests. Using a `ManualReset` avoids this issue. * deleted commented out code * Make updates to `FishUntil` (#5433) * Make updates to `FishUntil` Implement nitpicks from #5430 * Nitpick * cleaned up `FishUntilMessage` * removed `FluentAssertions` reference * fixed bad `TestKit.ReceiveTests` * Custom frame size computation support in Framing (#5444) * Custom frame size computation support in Framing * Speed up of framing spec * optimize `Props` `NewExpression` allocations (#5428) per #5416 (comment) Co-authored-by: Ismael Hamed <1279846+ismaelhamed@users.noreply.github.com>

Zetanova added 4 commits December 4, 2021 15:57

clean and seal props

551857f

make producer singleton

1cd6c39

add legacy support

f36a4e7

update api

d2b2508

Zetanova commented Dec 4, 2021

View reviewed changes

add producer type compare

f22a29c

Zetanova commented Dec 5, 2021

View reviewed changes

Zetanova added 7 commits December 5, 2021 08:14

fix props equals

3d3452e

fix test for invalid props expression

b9d6b07

fix racy test and testkit

8bb422b

fix thread sleep

07fc952

lower condition interval

171a6bc

fix await done async

bf73061

improve logging startup check

71bac15

Zetanova added 2 commits December 7, 2021 12:22

enable inline execution

a47d581

fix racy sync-over-async-over-sync call

af98f79

Zetanova added 3 commits December 7, 2021 14:03

add actor path address validation

57d9c43

fix racy starup in logging

292ec8a

fix DefaultAddress NRE

eb01377

add testkit log test on startup

fcf18c2

remove await log message hack

0171d99

Zetanova and others added 2 commits December 8, 2021 19:55

Merge branch 'dev' into perf-probs-emptyargs

c6aa54c

Merge branch 'dev' into perf-probs-emptyargs

5b0471a

Aaronontheweb self-requested a review December 13, 2021 13:40

Aaronontheweb added akka-actor api-change labels Dec 13, 2021

Aaronontheweb requested changes Dec 13, 2021

View reviewed changes

Aaronontheweb approved these changes Dec 13, 2021

View reviewed changes

Aaronontheweb added 2 commits December 13, 2021 11:53

Merge branch 'dev' into perf-probs-emptyargs

62182ce

Merge branch 'dev' into perf-probs-emptyargs

03151d4

Aaronontheweb enabled auto-merge (squash) December 14, 2021 01:33

to11mtm reviewed Dec 14, 2021

View reviewed changes

Aaronontheweb merged commit 12c26c2 into akkadotnet:dev Dec 14, 2021

Aaronontheweb added a commit to Aaronontheweb/akka.net that referenced this pull request Dec 14, 2021

optimize Props NewExpression allocations

b4f107c

per akkadotnet#5416 (comment)

Aaronontheweb mentioned this pull request Dec 14, 2021

optimize Props NewExpression allocations #5428

Merged

Zetanova mentioned this pull request Dec 16, 2021

Possible race condition in Akka.Cluster #5435

Closed

Aaronontheweb added a commit that referenced this pull request Dec 20, 2021

optimize Props NewExpression allocations (#5428)

42501db

per #5416 (comment)

Aaronontheweb added a commit to Aaronontheweb/akka.net that referenced this pull request Dec 20, 2021

optimize Props NewExpression allocations (akkadotnet#5428)

3be45c9

per akkadotnet#5416 (comment)

Arkatufus added a commit to Arkatufus/akka.net that referenced this pull request Dec 20, 2021

Revert akkadotnet#5416

0f30647

Aaronontheweb mentioned this pull request Dec 20, 2021

Revert #5416, broke DI backward compatibility #5454

Merged

Aaronontheweb pushed a commit that referenced this pull request Dec 20, 2021

Revert #5416 (#5454)

362dc2f

		{
		var t = ((SendOrPostCallback, object, ActorCellKeepingSynchronizationContext, ActorCell))s;

		var args = newExpression.GetArguments();
		args = args.Length > 0 ? args : NoArgs;

Refactor Props #5416

Refactor Props #5416

Conversation

Zetanova commented Dec 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zetanova commented Dec 5, 2021

Zetanova commented Dec 6, 2021

Aaronontheweb commented Dec 6, 2021

Zetanova commented Dec 6, 2021

Aaronontheweb commented Dec 6, 2021

Zetanova commented Dec 6, 2021

Zetanova commented Dec 7, 2021

Zetanova commented Dec 7, 2021

Zetanova commented Dec 7, 2021

Zetanova commented Dec 8, 2021 • edited Loading

Aaronontheweb commented Dec 8, 2021

Zetanova commented Dec 8, 2021

Zetanova commented Dec 8, 2021

Aaronontheweb commented Dec 13, 2021

Aaronontheweb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aaronontheweb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

to11mtm Dec 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zetanova Dec 15, 2021 • edited Loading

Choose a reason for hiding this comment

to11mtm Dec 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zetanova commented Dec 4, 2021 •

edited

Loading

Zetanova commented Dec 8, 2021 •

edited

Loading

to11mtm Dec 14, 2021 •

edited

Loading

Zetanova Dec 15, 2021 •

edited

Loading

to11mtm Dec 14, 2021 •

edited

Loading