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

Missing ConfigureAwait(false) #16

Open
danielmarbach opened this issue Dec 24, 2015 · 9 comments
Open

Missing ConfigureAwait(false) #16

danielmarbach opened this issue Dec 24, 2015 · 9 comments

Comments

@danielmarbach
Copy link

It wouldn't hurt to add ConfigureAwait(false) to each awat statement in the production code.

https://msdn.microsoft.com/en-us/magazine/jj991977.aspx

@randa1
Copy link
Contributor

randa1 commented Dec 25, 2015

Since the stream provider runs in Orleans context as part of grain code (PersistentStreamPullingAgent), adding ConfigureAwait(false) will actually cause the callback to break out of the Orleans scheduler and shouldn't be used:
http://dotnet.github.io/orleans/Advanced-Concepts/External-Tasks-and-Grains

If you think there's a specific place in which it should be used, please let us know.

@danielmarbach
Copy link
Author

A great. Didn't know that. After reading the explanation makes total sense. But I think when you call out into the kafka infra you explicitely want to use i.ex. the IO thread pool and only when you callback into the greans you want to schedule on the turn based one. But I'm not familiar enough with the orleans scheduler. Thoughts?

Am 25.12.2015 um 10:18 schrieb Ran Dahan [email protected]:

Since the stream provider runs in Orleans context as part of grain code (PersistentStreamPullingAgent), adding ConfigureAwait(false) will actually cause the callback to break out of the Orleans scheduler and shouldn't be used:
http://dotnet.github.io/orleans/Advanced-Concepts/External-Tasks-and-Grains

If you think there's a specific place in which it should be used, please let us know.


Reply to this email directly or view it on GitHub.

@danielmarbach
Copy link
Author

I read the doco twice now. After reading a second time I realized the best practice applies to the user code inside a grain. Does this also hold up for the infrastructure pieces? I doubt that you want to bombard the grain scheduler with IO calls. Same applied for schedulers which are dispatcher-aware (like WPF), you want to offload the heavy lifting in the infra to the IO scheduler and only join back when the continuations call into the grains again. But I might be wrong

Am 25.12.2015 um 10:18 schrieb Ran Dahan [email protected]:

Since the stream provider runs in Orleans context as part of grain code (PersistentStreamPullingAgent), adding ConfigureAwait(false) will actually cause the callback to break out of the Orleans scheduler and shouldn't be used:
http://dotnet.github.io/orleans/Advanced-Concepts/External-Tasks-and-Grains

If you think there's a specific place in which it should be used, please let us know.


Reply to this email directly or view it on GitHub.

@gabikliot
Copy link

OK, so to make it more clear:
if one is using await task.ConfigureAwait(false), the next line will run outside the context. But that applies only in that async level/scope. If you have a higher level scope that does not use ConfigureAwait(false), the context is still captured. Example:

// this code runs on Scheduler X - within some X context
await libraryCall();
// next line - this code still runs on Scheduler X in the same context

async Task libraryCall()
{
// ...
await otherCall().ConfigureAwait(false);
// context is lost here - it will run on the thread pool
}

What that means is that even if you are using await task.ConfigureAwait(false) inside a library (or any function), as long as your code calls await withoutConfigureAwait(false), YOUR code is fine, it still runs in the right context.

Re libraries: indeed, a good practice in the library code is to call ConfigureAwait(false) on ANY awaited task, for 2 reasons:

  1. save on context propagation - it will be potentiality cheaper not to propagate it.
  2. the library should not in general rely on the context set by the caller, and should not (by mistake) for example rely that it is set.

Now the question in this Kafka provider is what is the library and what is "your code that does require to be run single threaded" (what is user code and what are "infrastructure pieces").
Definitely, the Kafka client (not part of that repo) is a library and it should use ConfigureAwait(false). The code here, for example that line - I think in this case the _consumer implementation internally should use ConfigureAwait(false) (since _consumer IS the library), and the code here does not need to call Task.Run. The other reason why you might have used Task.Run is if the first lines inside _consumer.FetchLastOffset before the first await is doing some heavy compute, so you offload this from the Orleans scheduler. But if this is the case, than again I think the library should EXPLICITLY offload its heavy compute part from the caller's context into the thread pool. So it's the library responsibility, not the caller.

Another option is to make that call with ConfigureAwait:
CurrentOffset = await _consumer.FetchLastOffset().ConfigureAwait(false);
That would only be correct if you don't care that the rest of the code in KafkaQueueAdapterReceiver.Initialize may run on a different context. So in the Kafka provider, its your call what parts are the library and what parts are the "main code".

Frankly, I would stay on the safe side and if in doubt, NOT use ConfigureAwait(false) when not sure if a certain piece of code should or should not run in the Orleans context. Inside grains I would never use ConfigureAwait(false) (unless its a library). Inside provider code - I would do the same. Any part of the provider code that interacts with Orleans directly - I would not use ConfigureAwait(false) (or Task.Run for that matter). In this stream provider case some parts of the receiver code interact with the cache and Orleans' pulling agent also interacts with the cache. Thus, you don't even want to be in the business of thinking - can this code access the cache outside the context or not. Any code that is a pure library (code that does not interact with Orleans at all), I would definitely use ConfigureAwait(false).

Bottom line - I totally agree with @danielmarbach:
"I think when you call out into the kafka infra you explicitly want to use i.ex. the IO thread pool and only when you callback into the grains you want to schedule on the turn based one."

Hope that makes sense.

@daniellm
Copy link

Gabi,

You mentioned a couple of reasons in favor of using ConfigureAwait(false) in pure library code. I can think of a reason against: when a silo is under heavy load, using two schedulers that employ a total number threads greater than the number of cpu cores will cause context switches and a lower total throughput.

From our experience with our legacy asp.net stack when our production servers exceed 20%-30% cpu usage we're starting to see various timeout issues. Our hope is that our new Orleans stack will be able to sustain higher loads, if we stick to Orleans' scheduler, prevent thread starvations and reduce context switches. In light of that, do you think we're going overboard trying to stick with Orleans' scheduler? Do you believe reduced context switches aren't worth reduced parallelism and higher latency?

@gabikliot
Copy link

Great question!
Indeed, offloading too much to Thread Pool will result in using too much of Thread Pool threads, and this will lead to decreased throughput due to increased context switch overhead. Of course, it depends on how much you offload. I see it this way:
a) If you have to offload to Thread Pool, it is probably better to do it via ConfigureAwait(false) and not via Task.Run.
b) when you are writing a general purpose library, you basically have no other choice - you don't know how it will be used, and in what scheduler the calling code will be running, so you basically have to use ConfigureAwait(false). I actually did not see THE EXPLICIT .NET guideline about it anywhere (maybe it exists somewhere), but my general understanding is that this is how you are supposed to write libraries. I have seen a lot of libraries using this principle. In your case, the Kafka Client is such a general purpose library.
c) when you are writing code for Orleans - code that is specifically targeted to be run in Orleans only - such as your Kafka Stream Provider, it will indeed be better NOT to offload anything in that code to the Thread Pool and run on Orleans scheduler. It will result in less threads (less utilization of Thread Pool unlimited threads) and this will lead to better perf.

Interestingly, you wrote:

Do you believe reduced context switches aren't worth reduced parallelism and higher latency?

Surprisingly, it is actually the other way around! Reduced parallelism (sometimes, not always but sometimes), results in lower latency! Example:
imagine you have an 8 core machine and all your work is pure compute - no blocking IO, pure CPU. We compare 2 thread pools - one with 8 threads and one with 1000 threads. The threads just take work from the queue and execute it. The larger pool has more parallelism, but due to the increased context switching overhead, it will spend some part of CPU cycles in vain, resulting in less CPU cycles for real work, resulting in lower throughput. Since the throughput is lower, the latency (the time since the work item was put in the queue until it was done) is actually higher (the item will wait longer in the queue since the service rate is smaller). This holds in this particular system architecture, with shared queue and no blocking IO. In different architectures it might be different. But of course, Orleans is such an architecture. And we have indeed seen that reducing the number of threads (to a correct well tuned setting of course) results in lower latency!

You can also take a look in our technical report (http://research.microsoft.com/pubs/210931/Orleans-MSR-TR-2014-41.pdf) Section 5.1, Figure 5. It only discusses throughput, but is related to our discussion here.
We have been using a small thread pool internally and avoid offloading to ThreadPool as much as we can (there are a couple of places where we can't avoid it - Async Socket Receives are hard coded by the socket library to use IO completion ports on the Thread Pool - because it is a general purpose library!). The result is that we can run Orleans at 90%-95% CPU utilization with no hickups. We do it all the time.

So bottom line:
I think in your Kafka client library you have to use ConfigureAwait(false) but in the Kafka Stream Provider I recommend using neither ConfigureAwait(false) nor Task.Run (which is basically what I said in the comment above as well).

And just to restate the obvious here (for "liability reason", so I am not quoted latter on that Orleans "is so complicated"): our whole discussion here is since you guys in Gigya are super advanced users of Orleans. Our main target user is not an expert in Distributed Computing or Multi Threaded architectures, and for him the default out-of-the-box Orleans setting of single threaded Orleans scheduler in grains "just works", with great performance, and he does not need to even know about the existence of ConfigureAwait(false).

Sorry for such long answers. I like to be un-ambiguous.
Let me know if you have more questions.

@randa1
Copy link
Contributor

randa1 commented Dec 28, 2015

Gabi, thanks for taking the time to explain everything. No need to apologize for the long answers, they are very informative and the clarity is worth it!

We'll follow through with your suggestion both here and on our Kafka client.

When developing the stream provider we've relied on the Orleans docs page about tasks. Maybe it's worth to add this explanation to that page, or maybe create some advanced concepts page for it?

@gabikliot
Copy link

Sure, I will add a comment there about using configureAwait(false) in libraries.

@gabikliot
Copy link

I have added a section "Dealing with libraries" to http://dotnet.github.io/orleans/Advanced-Concepts/External-Tasks-and-Grains. Hopefully it will be more straightforward now.

You have no idea (or maybe you do) how lucky you/we are to have async/await in .NET. Other languages are so far behind! The productivity difference is enormous.

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

No branches or pull requests

4 participants