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

Update copy logic to use dedicated threads. #11272

Merged
merged 2 commits into from
Mar 10, 2025

Conversation

Erarndt
Copy link
Contributor

@Erarndt Erarndt commented Jan 13, 2025

Also updated some checks to help avoid hitting the file system if possible.

Fixes #

Context

The existing implementation does synchronous file copying on threadpool threads which can lead to starvation. Switching to dedicated threads to do synchronous copying helps keep the threadpool threads available for other tasks.

Changes Made

Testing

Notes

@rainersigwald
Copy link
Member

Just kinda generally, I'm surprised to hear about threadpool starvation in our worker nodes which aren't threadpool heavy. Have you observed this in practice?

@Erarndt
Copy link
Contributor Author

Erarndt commented Jan 13, 2025

Just kinda generally, I'm surprised to hear about threadpool starvation in our worker nodes which aren't threadpool heavy. Have you observed this in practice?

There is some sneaky utilization of the threadpool that ends up blocking enough to see the "hill climbing" algorithm start spinning up new threads. I've seen between 40 and 50 threadpool threads per MSBuild processes which is over 3x what I would expect. This change (#11275) has a larger impact than the copy code, but this contributes as well. There's also additional benefit to reducing the mix of I/O bound work and CPU bound work on the threadpool.

@JanKrivanek
Copy link
Member

Related to #11160

@Erarndt Erarndt marked this pull request as ready for review January 15, 2025 16:41
@JanProvaznik JanProvaznik self-requested a review January 16, 2025 10:28
Also updated some checks to help avoid hitting the file system if possible.
@JanProvaznik JanProvaznik force-pushed the dev/erarndt/updateCopyParallelism branch from d0500f7 to 868969a Compare January 16, 2025 10:29
Copy link
Member

@JanProvaznik JanProvaznik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable, please

  1. fix the test
  2. cleanup parallelism parameter
  3. if possible share some evidence of perf impact, I could not see it from looking at some local OrchardCore builds (albeit I did not achieve statistical significance)

@yuehuang010
Copy link
Contributor

Is there an option to use TaskCreationOptions.LongRunning flag? This should avoid the thread starvation problem.

@YuliiaKovalova YuliiaKovalova self-requested a review January 29, 2025 10:49
Copy link
Member

@SimaTian SimaTian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thank you.

@JanProvaznik JanProvaznik requested a review from Copilot February 27, 2025 13:10
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Overview

This PR updates the file copy logic to use dedicated threads to handle synchronous file copying, aiming to reduce threadpool starvation. Key changes include:

  • Adding a dedicated thread pool with static thread arrays and action queues for parallel copying.
  • Changing the parallelism configuration from an integer value to a boolean flag indicating whether to copy in parallel.
  • Updating unit tests to reflect the new boolean-based parallelism setting.

Reviewed Changes

File Description
src/Tasks/Copy.cs Introduces dedicated thread pool logic and revises parallel copy execution.
src/Framework/NativeMethods.cs Adjusts long path handling in GetFullPath using stack-allocated buffers.
src/Tasks.UnitTests/Copy_Tests.cs Updates tests to use the new boolean parallelism flag.

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/Tasks/Copy.cs:57

  • Review the use of static thread pool fields to ensure they don't interfere when multiple Copy task instances run concurrently; consider using instance-based thread pools or more robust synchronization if needed.
lock (_copyActionQueue)

Copy link
Member

@JanProvaznik JanProvaznik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Member

@YuliiaKovalova YuliiaKovalova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the contribution!
@AR-May , could you please measure the impact of the change ?

@JanProvaznik JanProvaznik added merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. and removed merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. labels Feb 27, 2025
@AR-May
Copy link
Member

AR-May commented Mar 10, 2025

Hi! Numbers:

ExecutableName RepoName Scenario Mean Median Variance Min Max EntriesNum
MSBuildBaseline OC inc-cold 33689.2 32643.3 11999119.992319642 29989.07 44322.37 20
MSBuildCompare OC inc-cold 32783.002440000004 31915.07 9742298.542851143 29628.52 40470.06 20
MSBuildBaseline OC re-cold 137929.7 136370.62884999998 47597265.67838404 126331.6 155266.6 20
MSBuildCompare OC re-cold 138762.7 137488.06410000002 157097140.4644246 125246.8 187710.1 20

I believe the performance difference is below our ability to detect it.

@AR-May AR-May merged commit 9fa9d80 into dotnet:main Mar 10, 2025
10 checks passed
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.

8 participants