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

[experiment] removed all instances of TaskContinuationOptions.AttachedToParent #1347

Closed
wants to merge 2 commits into from

Conversation

Aaronontheweb
Copy link
Member

I've removed all instances of TaskContinuationOptions.AttachedToParent from Akka.NET, in an experiment to determine if #1346 is behind some of the weird behavior in the multi-node testkit... And in general just to see if it has any impact on Akka.NET period.

Going to run this build several times on TeamCity and see how it performs.

@Aaronontheweb
Copy link
Member Author

Didn't have much of a visible impact on the multi-node testkit, Akka.Remote, or Akka.Persistence.

@rogeralsing
Copy link
Contributor

IMO, we should just throw out all the continuation optionsand use the default. ExecuteSync seems bad after reading the Orleans thread about sync and wait.

@JeffCyr
Copy link
Contributor

JeffCyr commented Oct 3, 2015

What was their issue with ExecuteSynchronously? I think this flag is good in your usage to execute small operation after a task without the overhead of (thread) context switch. You might get a performance penality by removing it.

There is one thing to know tough, this flag is a hint and not an obligation, the TaskScheduler can refuse to execute inline by returning false in its TryExecuteInline method (won't happen with the default scheduler).

An alternative to garantee that the continuation will execute sync after its parent task is to use a custom InlineTaskScheduler in the ContinueWith overloads. However be careful with this, anything using TaskScheduler.Current inside the continuation will use the InlineTaskScheduler.

@Aaronontheweb
Copy link
Member Author

@rogeralsing our uses of ExecuteSynchronously are fine internally, having reviewed most of them. Might be some in Akka.Persistence worth taking a look at

@rogeralsing
Copy link
Contributor

Should we do anything to this one? merge? or are you still working on it?

@Aaronontheweb
Copy link
Member Author

@rogeralsing IMHO, worth adding a throughput benchmark for Akka.NET core, async, and remote and then try this PR with and without it.

Leave it open for now and we'll get back to it once we've run some numbers

@rogeralsing
Copy link
Contributor

Still open?

@Aaronontheweb
Copy link
Member Author

@rogeralsing yeah - I should have a benchmark for this soon!

@JeffCyr
Copy link
Contributor

JeffCyr commented Dec 1, 2015

@Aaronontheweb
I fixed PipeTo in PR #1484 because it was directly harmful when called if TaskScheduler.Current is set to an ActorTaskScheduler. The other usage will also impact the ActorTaskScheduler so I suggest merging this PR asap.

I also realized that it would be better to explicitly use TaskScheduler.Default on all usage of Task.ContinueWith (unless you really want to be pushed in the current TaskScheduler which is rare). If you don't, even if TaskContinuationOptions.ExecuteSynchronously is set, the continuation may not execute synchronously if the current TaskScheduler returns false in its TryExecuteTaskInline method (like the ActorTaskScheduler if you are not already in the actor's context, it would be queued in the actor mailbox and potentially cause delays if the actor is busy).

@JeffCyr
Copy link
Contributor

JeffCyr commented Mar 12, 2016

PR stalled, replaced by up to date PR #1783

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants