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

Speed up azure service token provider #7527

Merged
merged 1 commit into from
Sep 12, 2019
Merged

Speed up azure service token provider #7527

merged 1 commit into from
Sep 12, 2019

Conversation

DavidParks8
Copy link
Contributor

The changes are small, though they will improve performance under high load because some states in the IL async state machine will no longer be generated.

@pakrym
Copy link
Contributor

pakrym commented Sep 9, 2019

Does this show up on profile? What performance improvement are you observing after this change?

@DavidParks8
Copy link
Contributor Author

If there is a custom profiling method in this repo, I don't know what the results are. However, I can tell you that my team did some extensive profiling of our own services last year in a big performance push, and I did profiling to test await without configure vs await with configureAwait(false), task.ContinueWith, task.ContinueWith while setting task continuation options, and just returning the task directly. Now, I don't remember the exact numbers because this was a while ago, but the speed order turned out to be as following from slowest to fastest:

  1. task.ContinueWith
  2. await without configure
  3. await with configureAwait(false)
  4. task.ContinueWith while setting continuation options to run synchronously
  5. return the task directly (I think this ended up being about 20% faster as compared to configureAwait(false)

The reasoning for await being slower was quite interesting. Await generates an async state machine in IL that also transfers the thread context between each case of the state machine. Think of each case as the block of code between two await calls. So, it makes sense that configureAwait(false) would be faster than plain await because you are telling the state machine to skip transfering the synchronization context for a particular case of the state machine. Then, it makes even more sense that task.ContinueWith while setting continuation options to run synchronously would be faster because it doesn't generate a state machine (due to the lack of await), and controlling the continuation makes it skip whatever made plain task.ContinueWith slow. Finally, returning the task directly is super fast because it doesn't generate a state machine, and it doesn't have any overhead from ContinueWith. It's like passing around just an object that happens to guarantee that something will eventually execute.

@DavidParks8
Copy link
Contributor Author

My team uses the following practices for tasks:
For functions with multiple await statements: If you need log correlation within a function, then ConfigureAwait(true) or don't ConfigureAwait, otherwise ConfigureAwait(false) whenever possible.

For functions that do a single Task operation, if the operation is the last line of the function, then return it directly without awaiting, otherwise ConfigureAwait(false). If you want to catch errors, then do the following:

try
{
    // failing to await in this case would mean returning the task and never hitting the catch block
    return await operation.ConfigureAwait(false);
}
catch (Exception e) 
{
 // handle error
}

Then, for extremely high performance with multiple tasks (think the performance requirements of a gateway), we might use ContinueWith and ensure the continuation runs synchronously. It all depends on the maintenance vs performance characteristics and the tradeoffs.

@pakrym
Copy link
Contributor

pakrym commented Sep 9, 2019

I'm not arguing that returning the task directly is faster, as you've said it avoids allocation and a state machine creation but while it's relatively faster the absolute time any of this options takes is very low so the optimization matters only on major hot paths. We can't be going around changing all return awaits to returns because most of the time it doesn't matter.

What I'm wondering is how did you decide that these specific methods need optimization? The other reason I'm asking is that if these methods do show up on the hot paths with a considerable impact there is a LOT of low hanging fruit in terms of optimizations here (avoiding allocations mostly).

@DavidParks8
Copy link
Contributor Author

Ah, the answer is quite simple. My division (IAM) started using this class. While seeing what it does internally, I saw this tiny change that could be made for the better.

@DavidParks8
Copy link
Contributor Author

And yes, this class will be used on the hot path for our services that use app-only permissions and need to obtain a token for every incoming request.

@DavidParks8
Copy link
Contributor Author

I'm not authorized to merge. @pakrym, what do I need to do to get this merged?

@pakrym
Copy link
Contributor

pakrym commented Sep 12, 2019

I'll do it. Thank you for contribution.

@pakrym pakrym merged commit 884edf1 into Azure:master Sep 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants