Fix ThreadPool tests for uapaot. #21049
Conversation
| @@ -345,6 +345,7 @@ public static void TaskMethodBuilderT_TaskIsCached() | |||
| } | |||
|
|
|||
| [Fact] | |||
| [ActiveIssue("TFS 450361 - Codegen optimization issue", TargetFrameworkMonikers.UapAot)] | |||
| public static void TaskMethodBuilder_UsesCompletedCache() | |||
There was a problem hiding this comment.
Weren't these already disabled in #21002?
There was a problem hiding this comment.
I reused the branch from the previous PR and GitHub included the previous commit. Changes in this file have been already merged; please ignore them.
There was a problem hiding this comment.
Can you please rebase it on top of the latest from master?
There was a problem hiding this comment.
@stephentoub Rebased.
| }; | ||
|
|
||
| // ThreadPool.SetMinThreads returns false on UapAot |
There was a problem hiding this comment.
We still want to verify that this returns true on the platforms where it should. And isn't it a bug that it returns false on UapAot right now?
There was a problem hiding this comment.
UapAot uses the default Win32 thread pool, which does not allow setting min/max numbers. I suggested using a custom Win32 thread pool to allow setting min/max numbers, however, that idea was not approved. We need evidence that these settings may be important for real-world applications.
There was a problem hiding this comment.
I think it's still useful to allow setting min/max number of threads (sometimes the scenario favors running more threads than the processor count to allow short work to run and complete while relatively long CPU-bound work is holding up some threads). But it probably isn't a good idea to hook those into SetMinThreads / SetMaxThreads since they are extensively used to work around the issues of the CoreCLR thread pool, and the Windows thread pool would not have many of those issues. Perhaps it can be exposed later as a config option or a different API or something.
There was a problem hiding this comment.
I think we may simply delete this ThreadPool.SetMinThreads call; the test does not depend on it.
There was a problem hiding this comment.
The test is testing concurrent calls to SetMinThreads (and thread pool initialization as part of that, based on CoreCLR implementation). In hindsight, the CoreCLR repo may be a better location for this test. In any case, for now if there's no issue with the test I would suggest keeping it as it was.
There was a problem hiding this comment.
@kouvel Oh, I completely missed that it is testing concurrent calls to SetMinThreads. In this case I will just disable it for UapAot.
| Assert.True(minw > 0); | ||
| Assert.True(minc > 0); | ||
| Assert.True(minw >= 0); | ||
| Assert.True(minc >= 0); |
There was a problem hiding this comment.
In what situation is it valid that these return 0?
There was a problem hiding this comment.
I think these should return Environment.ProcessorCount if min/max thread changes are not supported in uapaot
There was a problem hiding this comment.
For instance, you may call ThreadPool.SetMinThreads(0, 0) on desktop CLR. GetMinThreads will return zeroes after that. At present UapAot returns zeroes, and I don't see why zeroes should be treated as invalid.
There was a problem hiding this comment.
@kouvel Returning Environment.ProcessorCount would be technically a lie. We may still do that if we had evidence that would improve compatibility with real-world apps.
There was a problem hiding this comment.
Oh yea that's true, I was thinking max...
|
cc: @kouvel |
| }; | ||
|
|
||
| // ThreadPool.SetMinThreads returns false on UapAot | ||
| ThreadPool.SetMinThreads(processorCount, processorCount); |
There was a problem hiding this comment.
The purpose of this test is to call SetMinThreads concurrently from different threads, so SetMinThreads needs to be called from the background thread. If SetMinThreads returns false on uapaot, what is wrong with this test that requires a change?
There was a problem hiding this comment.
The test was asserting that SetMinThreads returned true. I will revert changes to this test and disable it for UapAot.
No description provided.