Skip to content

Conversation

@jonathanpeppers
Copy link
Member

@jonathanpeppers jonathanpeppers commented Jul 28, 2021

Fixes: https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_workitems/edit/1360410
Context: https://devblogs.microsoft.com/premier-developer/limiting-concurrency-for-faster-and-more-responsive-apps/

A work item came in from VS where AOT compilation is causing
responsiveness issues. The culprit came down to some of the
TaskScheduler and TaskCreationOptions settings in
AsyncTaskExtensions.

@davkean's recommendations are:

This code needs to do two things:

  1. It needs to pass TaskCreationOptions.LongRunning, so that we
    don't burn queues meant for short lived tasks.
  2. It needs to limit how many tasks it starts at once, to avoid CPU
    contention and reduce the number of threads that we end up
    burning.

Looking at AsyncTaskExtensions usage, it would happen during regular
builds as well, because aapt2-related tasks use these.

To improve these, I'm changing:

  • New overloads to pass in int maxConcurrencyLevel and
    TaskCreationOptions.
  • These default to Environment.ProcessorCount * 2 and LongRunning.

When this change lands, we should potentially pass in
$(Aapt2DaemonMaxInstanceCount) where appropriate for
maxConcurrencyLevel.

I wrote a few tests to just check general sanity of
AsyncTaskExtensions. We didn't have any.

Context: https://devblogs.microsoft.com/premier-developer/limiting-concurrency-for-faster-and-more-responsive-apps/

A work item came in from VS where AOT compilation is causing
responsiveness issues. The culprit came down to some of the
`TaskScheduler` and `TaskCreationOptions` settings in
`AsyncTaskExtensions`.

@davkean's recommendations are:

> This code needs to do two things:
> 1) It needs to pass `TaskCreationOptions.LongRunning`, so that we
>    don't burn queues meant for short lived tasks.
> 2) It needs to limit how many tasks it starts at once, to avoid CPU
>    contention and reduce the number of threads that we end up
>    burning.

Looking at `AsyncTaskExtensions` usage, it would happen during regular
builds as well, because aapt2-related tasks use these.

To improve these, I'm changing:

* New overloads to pass in `int maxConcurrencyLevel` and
  `TaskCreationOptions`.
* These default to `Environment.ProcessorCount * 2` and `LongRunning`.

When this change lands, we should potentially pass in
`$(Aapt2DaemonMaxInstanceCount)` where appropriate for
`maxConcurrencyLevel`.

I wrote a few tests to just check general sanity of
`AsyncTaskExtensions`. We didn't have any.
@dellis1972
Copy link
Contributor

When this change lands, we should potentially pass in
$(Aapt2DaemonMaxInstanceCount) where appropriate for
maxConcurrencyLevel.

We should probably have separate properties for each, but unify them so the user can set both at once, or tweak each value.
Something like

<AndroidMaxParallelBuildCount>6</AndroidMaxParallelBuildCount>
<Aapt2DaemonMaxInstanceCount Condition=" '$(Aapt2DaemonMaxInstanceCount)' == '' ">$(AndroidMaxParallelBuildCount )</Aapt2DaemonMaxInstanceCount>
<AotMaxBuildThreadCount Condition=" '$(AotMaxBuildThreadCount)' == '' ">$(AndroidMaxParallelBuildCount )</AotMaxBuildThreadCount >

(We'd need to work on the naming :)).

Mind you , I not sure if anyone has ever used this property yet, apart from that one guy who one had 1 CPU and we ended up hanging the build because we calculated (at that time) ProcessorCount -1 and started 0 daemons 🤦

@jonathanpeppers
Copy link
Member Author

Since they can't control AOT parallelization now, I think we could probably just choose a default here and not need to create new properties.

@dellis1972 dellis1972 merged commit f9c1b0d into dotnet:main Jul 29, 2021
@jonathanpeppers jonathanpeppers deleted the asynctaskextensions branch September 1, 2021 16:22
@jonathanpeppers
Copy link
Member Author

I saw this API that @radical was using here:

https://github.com/dotnet/runtime/pull/59391/files#diff-2f7f7e10f4474e62ffedeb8c4cca26556078e3b977b1bd73b1ecd7ec7c0fda04R384

if (BuildEngine is IBuildEngine9 be9)
    allowedParallelism = be9.RequestCores(allowedParallelism);

Maybe we should actually use this?

@dellis1972
Copy link
Contributor

Interesting. We'd need to find out if that is for MSBuild Project Parallelism or if we can use it for our Background Thread stuff.

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.

3 participants