Bugfix/30024 problem with concurrency type descriptor getconverter - #85156

Closed
Maximys wants to merge 7 commits into
dotnet:mainfrom
Maximys:bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter
Closed

Bugfix/30024 problem with concurrency type descriptor getconverter#85156
Maximys wants to merge 7 commits into
dotnet:mainfrom
Maximys:bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter

Conversation

@Maximys

@MaximysMaximys commented Apr 21, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

This PR fix#30024 .
Problem was linked with race condition inside private method System.ComponentModel.TypeDescriptor.CheckDefaultProvider(Type).
I think, it'll better to change base type of WeakHashtable from Hashtable to ConcurrentDictionary<TKey,TValue> and use method GetOrAdd(TKey, Func<TKey,TValue>), but this changes can be very dangerous and I'm afraid to do them.

@ghostghost added area-System.ComponentModel community-contribution Indicates that the PR has been added by a community member labels Apr 21, 2023
@ghost

ghost commented Apr 21, 2023

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-componentmodel
See info in area-owners.md if you want to be subscribed.

Issue Details

This PR fix #30024 .
Problem was linked with race condition inside private method System.ComponentModel.TypeDescriptor.CheckDefaultProvider(Type).
I think, it'll better to change base type of WeakHashtable from Hashtable to ConcurrentDictionary<TKey,TValue>, but this changes can be very dangerous and I'm afraid to do them.

Author:Maximys
Assignees:-
Labels:

area-System.ComponentModel

Milestone:-

@Maximys
Maximysforce-pushed the bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter branch from f3e9d88 to 4693f40CompareApril 21, 2023 11:05
This was referenced May 2, 2023
Closed
Closed
@Maximys
Maximysforce-pushed the bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter branch from 0f46857 to e9e137eCompareMay 5, 2023 06:48
stephentoub
stephentoub reviewed May 6, 2023
View reviewed changes
Comment threadsrc/libraries/System.ComponentModel.TypeConverter/tests/TypeDescriptionProviderTests.cs Outdated
stephentoub
stephentoub requested changes May 6, 2023
View reviewed changes

@stephentoubstephentoub left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

At quick glance, this doesn't look like a valid change.

The NodeFor method calls CheckDefaultProvider which takes the lock on s_internalSyncObject, and AddProvider calls NodeFor while holding a lock on s_providerTable. That means while holding s_providerTable the implementation can then take a lock on s_internalSyncObject. This change, though, is extending the lock in CheckDefaultProvider such that while holding the lock on s_internalSyncObject, it calls AddProvider, which will try to take a lock on s_providerTable. That means two threads could now deadlock: one holds the lock on s_providerTable and tries to acquire the lock on s_internalSyncObject, and the other holds the lock on s_internalSyncObject and tries to acquire the lock on s_providerTable.

stephentoub

This comment was marked as duplicate.

Sign in to view

@ghostghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label May 6, 2023
@Maximys
Maximysforce-pushed the bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter branch from e9e137e to 92254efCompareMay 13, 2023 16:48
@ghostghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label May 13, 2023
@Maximys

Maximys commented May 14, 2023

Copy link
Copy Markdown
ContributorAuthor

@stephentoub , thanks for your comment and attention. I had fixed this problem by ReaderWriterLockSlim and made amend of my commits.

@Maximys

Maximys commented May 22, 2023

Copy link
Copy Markdown
ContributorAuthor

@stephentoub ?

steveharter
steveharter approved these changes Jun 26, 2023
View reviewed changes

@stevehartersteveharter left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The latest rebase\changes look like they address the feedback from @stephentoub.

@stephentoub

stephentoub commented Jun 26, 2023

Copy link
Copy Markdown
Member

The latest rebase\changes look like they address the feedback from @stephentoub.

I've not re-reviewed to determine whether there might still be the potential for deadlocks.

Also, what's the perf impact? It's now taking a reader lock on a path that wasn't previously locked.

@Maximys

Maximys commented Jun 27, 2023

Copy link
Copy Markdown
ContributorAuthor

The latest rebase\changes look like they address the feedback from @stephentoub.

I've not re-reviewed to determine whether there might still be the potential for deadlocks.

Also, what's the perf impact? It's now taking a reader lock on a path that wasn't previously locked.

@stephentoub , could you provide some info about steps for perfomance measurement for TypeDescriptor inside current repository? I usually prefer to use BenchmarkDotNet inside console application, but I don't know how to apply this way for profiling code inside .NET Runtime.

@steveharter

steveharter commented Jun 27, 2023

Copy link
Copy Markdown
Contributor

@stephentoub , could you provide some info about steps for perfomance measurement for TypeDescriptor inside current repository? I usually prefer to use BenchmarkDotNet inside console application, but I don't know how to apply this way for profiling code inside .NET Runtime.

We do have official benchmarks, but even those call public APIs. Would using variants of your tests with BenchmarkDotNet measure the extra read lock? If the test is very fast, you may want an inner loop within the benchmark method.

@Maximys

Maximys commented Jun 28, 2023

Copy link
Copy Markdown
ContributorAuthor

@stephentoub , could you provide some info about steps for perfomance measurement for TypeDescriptor inside current repository? I usually prefer to use BenchmarkDotNet inside console application, but I don't know how to apply this way for profiling code inside .NET Runtime.

We do have official benchmarks, but even those call public APIs. Would using variants of your tests with BenchmarkDotNet measure the extra read lock? If the test is very fast, you may want an inner loop within the benchmark method.

I had create special branch bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter(NEW), where I trying to add required tests. After that, I had build Runtime by
build.cmd clr+libs+libs.tests -rc release -lc release
as it's described here. And then I had trying to use
dotnet run -c Release -f net8.0 --filter "FullyQualifiedName=System.ComponentModel.Tests.PerformanceTests.GetConverter_ByMultithread_New" \ --corerun C:\runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe
command for run one of my tests, but output of this command is:

C:\Program Files\dotnet
C:\Program Files\dotnet\sdk\8.0.100-preview.5.23303.2\Sdks\Microsoft.NET.Sdk\targets\Microsoft.PackageDependencyResolution.targets(266,5): error NETSDK1005: Assets file 'C:\runtime\artifacts\obj\tasks\project.assets.json' doesn't have a target for 'net8.0'. Ensure that restore has run and that you have included 'net8.0' in the TargetFrameworks for your project.

And I can't figure out how to fix this problem. May be somebody could explain me steps for running BenchmarkDotNet inside current repository, as I asked earlier.

@Maximys

Maximys commented Jun 28, 2023

Copy link
Copy Markdown
ContributorAuthor

If I run already existed perfomance tests from dotnet/performance by
dotnet run -c Release -f net8.0 --filter "System.ComponentModel.Tests.Perf_TypeDescriptorTests.GetConverter" \ --corerun "C:\runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe" "C:\runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe"

command, then I receive next result:

// * Summary *
BenchmarkDotNet=v0.13.2.2052-nightly, OS=Windows 10 (10.0.19045.3086)
AMD Ryzen 5 2600, 1 CPU, 12 logical and 6 physical cores
.NET SDK=8.0.100-preview.5.23303.2
[Host] : .NET 8.0.0 (8.0.23.28008), X64 RyuJIT AVX2
Job-XYPMDP : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT AVX2
Job-FNPGHD : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT AVX2
PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:EnableUnsafeBinaryFormatterSerialization=true IterationTime=250.0000 ms
MaxIterationCount=20 MinIterationCount=15 WarmupCount=1
| Method | Job | Toolchain | typeToConvert | Mean | Error | StdDev | Median | Min | Max | Ratio | RatioSD | Allocated | Alloc Ratio |
|------------- |----------- |------------------------------------------------------------------------------------------------------------------------- |--------------------- |---------:|--------:|--------:|---------:|---------:|---------:|------:|--------:|----------:|------------:|
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassIDerived | 146.5 ns | 0.86 ns | 0.72 ns | 146.3 ns | 145.6 ns | 147.9 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassIDerived | 112.6 ns | 0.30 ns | 0.28 ns | 112.6 ns | 112.2 ns | 113.1 ns | 0.77 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassWithNoConverter | 147.3 ns | 0.45 ns | 0.43 ns | 147.3 ns | 146.5 ns | 148.1 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassWithNoConverter | 111.0 ns | 0.17 ns | 0.15 ns | 111.0 ns | 110.7 ns | 111.4 ns | 0.75 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | DerivedClass | 200.3 ns | 1.90 ns | 1.78 ns | 200.4 ns | 196.9 ns | 203.4 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | DerivedClass | 161.5 ns | 5.24 ns | 5.82 ns | 160.1 ns | 155.6 ns | 177.6 ns | 0.81 | 0.03 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | IDerived | 138.1 ns | 0.27 ns | 0.25 ns | 138.0 ns | 137.8 ns | 138.5 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | IDerived | 102.1 ns | 0.08 ns | 0.07 ns | 102.1 ns | 102.1 ns | 102.3 ns | 0.74 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeEnum | 225.1 ns | 0.80 ns | 0.74 ns | 225.3 ns | 223.8 ns | 226.1 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeEnum | 185.4 ns | 1.26 ns | 0.98 ns | 185.2 ns | 184.4 ns | 188.0 ns | 0.82 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Enum | 197.6 ns | 1.10 ns | 0.97 ns | 197.4 ns | 196.2 ns | 199.9 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Enum | 163.6 ns | 4.30 ns | 4.78 ns | 164.2 ns | 155.8 ns | 170.7 ns | 0.83 | 0.02 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Guid | 203.0 ns | 3.68 ns | 3.44 ns | 204.4 ns | 195.4 ns | 209.0 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Guid | 158.9 ns | 3.07 ns | 2.87 ns | 158.0 ns | 155.8 ns | 166.4 ns | 0.78 | 0.02 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32 | 199.0 ns | 3.99 ns | 4.60 ns | 200.4 ns | 191.5 ns | 205.8 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32 | 156.9 ns | 2.71 ns | 2.40 ns | 156.8 ns | 153.7 ns | 162.3 ns | 0.79 | 0.02 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeValueType? | 197.0 ns | 0.80 ns | 0.74 ns | 197.2 ns | 195.3 ns | 197.8 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeValueType? | 161.3 ns | 3.74 ns | 4.31 ns | 160.7 ns | 154.8 ns | 168.4 ns | 0.82 | 0.02 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32? | 207.1 ns | 5.69 ns | 6.32 ns | 206.5 ns | 196.9 ns | 220.8 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32? | 160.0 ns | 2.75 ns | 2.57 ns | 159.9 ns | 156.3 ns | 165.0 ns | 0.77 | 0.02 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | String | 144.4 ns | 0.44 ns | 0.39 ns | 144.3 ns | 143.7 ns | 145.2 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | String | 105.9 ns | 0.09 ns | 0.08 ns | 105.9 ns | 105.8 ns | 106.0 ns | 0.73 | 0.00 | - | NA |

that is, performance of TypeDescriptor.GetConverter has degraded by about 20%

@steveharter

steveharter commented Jun 28, 2023

Copy link
Copy Markdown
Contributor

that is, performance of TypeDescriptor.GetConverter has degraded by about 20%

That seems reasonable to me.

@Maximys

Maximys commented Jun 30, 2023

Copy link
Copy Markdown
ContributorAuthor

@stephentoub , also lets look to one more branch of my repository: 30024-problem-with-concurrency-typeDescriptor-getconverter(NEW). It contains one more little optimization which provide next result:

// * Summary *
BenchmarkDotNet=v0.13.2.2052-nightly, OS=Windows 10 (10.0.19045.3155)
AMD Ryzen 5 2600, 1 CPU, 12 logical and 6 physical cores
.NET SDK=8.0.100-preview.5.23303.2
[Host] : .NET 8.0.0 (8.0.23.28008), X64 RyuJIT AVX2
Job-ICGKTF : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT AVX2
Job-LAEOHV : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT AVX2
PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:EnableUnsafeBinaryFormatterSerialization=true IterationTime=250.0000 ms
MaxIterationCount=20 MinIterationCount=15 WarmupCount=1
| Method | Job | Toolchain | typeToConvert | Mean | Error | StdDev | Median | Min | Max | Ratio | RatioSD | Allocated | Alloc Ratio |
|------------- |----------- |------------------------------------------------------------------------------------------------------------------------- |--------------------- |---------:|--------:|--------:|---------:|---------:|---------:|------:|--------:|----------:|------------:|
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassIDerived | 123.7 ns | 0.41 ns | 0.34 ns | 123.6 ns | 123.4 ns | 124.6 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassIDerived | 109.3 ns | 0.08 ns | 0.06 ns | 109.4 ns | 109.2 ns | 109.5 ns | 0.88 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassWithNoConverter | 125.5 ns | 0.33 ns | 0.31 ns | 125.5 ns | 124.8 ns | 125.9 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassWithNoConverter | 112.4 ns | 0.24 ns | 0.21 ns | 112.4 ns | 111.8 ns | 112.6 ns | 0.90 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | DerivedClass | 184.7 ns | 1.69 ns | 1.50 ns | 184.2 ns | 182.6 ns | 187.5 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | DerivedClass | 161.5 ns | 1.68 ns | 1.57 ns | 160.9 ns | 159.4 ns | 164.5 ns | 0.88 | 0.01 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | IDerived | 119.7 ns | 0.51 ns | 0.47 ns | 119.7 ns | 118.8 ns | 120.4 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | IDerived | 102.3 ns | 0.07 ns | 0.06 ns | 102.3 ns | 102.1 ns | 102.4 ns | 0.85 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeEnum | 204.4 ns | 0.52 ns | 0.48 ns | 204.4 ns | 203.3 ns | 205.0 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeEnum | 183.8 ns | 0.20 ns | 0.17 ns | 183.8 ns | 183.3 ns | 184.0 ns | 0.90 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Enum | 183.8 ns | 1.75 ns | 1.64 ns | 184.4 ns | 180.3 ns | 186.0 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Enum | 162.6 ns | 2.01 ns | 1.88 ns | 162.3 ns | 159.9 ns | 166.3 ns | 0.88 | 0.01 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Guid | 185.3 ns | 0.85 ns | 0.79 ns | 185.5 ns | 183.2 ns | 186.1 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Guid | 162.5 ns | 0.90 ns | 0.85 ns | 162.7 ns | 159.9 ns | 163.4 ns | 0.88 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32 | 181.4 ns | 1.72 ns | 1.61 ns | 181.8 ns | 178.5 ns | 183.6 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32 | 161.1 ns | 1.30 ns | 1.22 ns | 160.9 ns | 158.7 ns | 162.7 ns | 0.89 | 0.01 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeValueType? | 179.8 ns | 2.63 ns | 2.46 ns | 180.2 ns | 175.7 ns | 183.4 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeValueType? | 160.5 ns | 2.12 ns | 1.98 ns | 160.3 ns | 157.5 ns | 163.9 ns | 0.89 | 0.02 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32? | 179.4 ns | 2.73 ns | 2.55 ns | 179.5 ns | 175.8 ns | 184.2 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32? | 157.7 ns | 1.77 ns | 1.65 ns | 157.1 ns | 155.4 ns | 160.5 ns | 0.88 | 0.01 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | String | 124.0 ns | 0.49 ns | 0.46 ns | 124.2 ns | 123.5 ns | 124.6 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | String | 109.4 ns | 0.06 ns | 0.05 ns | 109.4 ns | 109.3 ns | 109.4 ns | 0.88 | 0.00 | - | NA |

As you can see, perfomance degradation has decreased to 10% after commit 92a2d245958ce41e05398580c0ea45419e05effe. May be it's ok? What do you think?

@steveharter
steveharter requested a review from buyaa-nJuly 5, 2023 17:19
@Maximys
Maximysforce-pushed the bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter branch from 92254ef to 1f20655CompareAugust 4, 2023 04:25
steveharter
steveharter reviewed Aug 4, 2023
View reviewed changes
Comment threadsrc/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs Outdated
Comment threadsrc/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs Outdated
Comment threadsrc/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs
Comment threadsrc/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs
@steveharter
steveharter self-requested a review August 7, 2023 15:11
@steveharter

steveharter commented Aug 7, 2023

Copy link
Copy Markdown
Contributor

@stephentoub - PTAL again thanks

@steveharter

steveharter commented Aug 9, 2023

Copy link
Copy Markdown
Contributor

Closing this PR for v8 due to risk + schedule; ideally should be done early in the 9.0 branch. There is risk in this PR and we need more time to verify this than what we have left in v8 -- RC1 is almost done.

@Maximys thanks for this PR.

cc @JeremyKuhne - thoughts on fixing this? This issue also occurs in .NET Framework to my knowledge.

@ghostghost locked as resolved and limited conversation to collaborators Sep 8, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@stephentoubstephentoubAwaiting requested review from stephentoub
@buyaa-nbuyaa-nAwaiting requested review from buyaa-n
1 more reviewer
@steveharterstevehartersteveharter approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-System.ComponentModelcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Concurrency issue in TypeDescriptor.GetConverter(type)

4 participants

@Maximys@stephentoub@steveharter@buyaa-n
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Bugfix/30024 problem with concurrency type descriptor getconverter - #85156
Closed
Maximys wants to merge 7 commits into
dotnet:mainfrom 
Maximys:bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter
Closed
Bugfix/30024 problem with concurrency type descriptor getconverter#85156
Maximys wants to merge 7 commits into
dotnet:mainfrom 
Maximys:bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter
Conversation
@Maximys
@MaximysMaximys
commented
Apr 21, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
This PR fix#30024 .

Problem was linked with race condition inside private method System.ComponentModel.TypeDescriptor.CheckDefaultProvider(Type).

I think, it'll better to change base type of WeakHashtable from Hashtable to ConcurrentDictionary<TKey,TValue> and use method GetOrAdd(TKey, Func<TKey,TValue>), but this changes can be very dangerous and I'm afraid to do them.
@ghostghost
added

area-System.ComponentModel

community-contribution
Indicates that the PR has been added by a community member
labels
Apr 21, 2023
@ghost
ghost
commented
Apr 21, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-componentmodel

See info in area-owners.md if you want to be subscribed.
Issue Details
This PR fix #30024 .

Problem was linked with race condition inside private method System.ComponentModel.TypeDescriptor.CheckDefaultProvider(Type).

I think, it'll better to change base type of WeakHashtable from Hashtable to ConcurrentDictionary<TKey,TValue>, but this changes can be very dangerous and I'm afraid to do them.
Author:Maximys
Assignees:-
Labels:
area-System.ComponentModel
Milestone:-
@Maximys
Maximysforce-pushed
the
bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter
branch
from
f3e9d88 to
4693f40CompareApril 21, 2023 11:05

This was referenced May 2, 2023

Closed


Closed

@Maximys
Maximysforce-pushed
the
bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter
branch
from
0f46857 to
e9e137eCompareMay 5, 2023 06:48
stephentoub
stephentoub
reviewed
May 6, 2023
View reviewed changes
Comment threadsrc/libraries/System.ComponentModel.TypeConverter/tests/TypeDescriptionProviderTests.cs
Outdated

stephentoub
stephentoub
requested changes
May 6, 2023
View reviewed changes
@stephentoubstephentoub
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

At quick glance, this doesn't look like a valid change.
The NodeFor method calls CheckDefaultProvider which takes the lock on s_internalSyncObject, and AddProvider calls NodeFor while holding a lock on s_providerTable. That means while holding s_providerTable the implementation can then take a lock on s_internalSyncObject. This change, though, is extending the lock in CheckDefaultProvider such that while holding the lock on s_internalSyncObject, it calls AddProvider, which will try to take a lock on s_providerTable. That means two threads could now deadlock: one holds the lock on s_providerTable and tries to acquire the lock on s_internalSyncObject, and the other holds the lock on s_internalSyncObject and tries to acquire the lock on s_providerTable.
stephentoub

This comment was marked as duplicate.

Sign in to view
@ghostghost
added
the 
needs-author-action
An issue or pull request that requires more info or actions from the author.
label
May 6, 2023
@Maximys
Maximysforce-pushed
the
bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter
branch
from
e9e137e to
92254efCompareMay 13, 2023 16:48
@ghostghost
removed
the 
needs-author-action
An issue or pull request that requires more info or actions from the author.
label
May 13, 2023
@Maximys
Maximys
commented
May 14, 2023

Copy link


Copy Markdown

ContributorAuthor
@stephentoub , thanks for your comment and attention. I had fixed this problem by ReaderWriterLockSlim and made amend of my commits.
@Maximys
Maximys
commented
May 22, 2023

Copy link


Copy Markdown

ContributorAuthor
@stephentoub ?
steveharter
steveharter
approved these changes
Jun 26, 2023
View reviewed changes
@stevehartersteveharter
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

The latest rebase\changes look like they address the feedback from @stephentoub.
@stephentoub
stephentoub
commented
Jun 26, 2023

Copy link


Copy Markdown

Member
The latest rebase\changes look like they address the feedback from @stephentoub.
I've not re-reviewed to determine whether there might still be the potential for deadlocks.
Also, what's the perf impact? It's now taking a reader lock on a path that wasn't previously locked.
@Maximys
Maximys
commented
Jun 27, 2023

Copy link


Copy Markdown

ContributorAuthor
The latest rebase\changes look like they address the feedback from @stephentoub.
I've not re-reviewed to determine whether there might still be the potential for deadlocks.
Also, what's the perf impact? It's now taking a reader lock on a path that wasn't previously locked.
@stephentoub , could you provide some info about steps for perfomance measurement for TypeDescriptor inside current repository? I usually prefer to use BenchmarkDotNet inside console application, but I don't know how to apply this way for profiling code inside .NET Runtime.
@steveharter
steveharter
commented
Jun 27, 2023

Copy link


Copy Markdown

Contributor
@stephentoub , could you provide some info about steps for perfomance measurement for TypeDescriptor inside current repository? I usually prefer to use BenchmarkDotNet inside console application, but I don't know how to apply this way for profiling code inside .NET Runtime.
We do have official benchmarks, but even those call public APIs. Would using variants of your tests with BenchmarkDotNet measure the extra read lock? If the test is very fast, you may want an inner loop within the benchmark method.
@Maximys
Maximys
commented
Jun 28, 2023

Copy link


Copy Markdown

ContributorAuthor
@stephentoub , could you provide some info about steps for perfomance measurement for TypeDescriptor inside current repository? I usually prefer to use BenchmarkDotNet inside console application, but I don't know how to apply this way for profiling code inside .NET Runtime.
We do have official benchmarks, but even those call public APIs. Would using variants of your tests with BenchmarkDotNet measure the extra read lock? If the test is very fast, you may want an inner loop within the benchmark method.
I had create special branch bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter(NEW), where I trying to add required tests. After that, I had build Runtime by
build.cmd clr+libs+libs.tests -rc release -lc release

as it's described here. And then I had trying to use
dotnet run -c Release -f net8.0 --filter "FullyQualifiedName=System.ComponentModel.Tests.PerformanceTests.GetConverter_ByMultithread_New" \ --corerun C:\runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe

command for run one of my tests, but output of this command is:
C:\Program Files\dotnet
C:\Program Files\dotnet\sdk\8.0.100-preview.5.23303.2\Sdks\Microsoft.NET.Sdk\targets\Microsoft.PackageDependencyResolution.targets(266,5): error NETSDK1005: Assets file 'C:\runtime\artifacts\obj\tasks\project.assets.json' doesn't have a target for 'net8.0'. Ensure that restore has run and that you have included 'net8.0' in the TargetFrameworks for your project.

And I can't figure out how to fix this problem. May be somebody could explain me steps for running BenchmarkDotNet inside current repository, as I asked earlier.
@Maximys
Maximys
commented
Jun 28, 2023

Copy link


Copy Markdown

ContributorAuthor
If I run already existed perfomance tests from dotnet/performance by
dotnet run -c Release -f net8.0 --filter "System.ComponentModel.Tests.Perf_TypeDescriptorTests.GetConverter" \ --corerun "C:\runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe" "C:\runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe"
command, then I receive next result:
// * Summary *
BenchmarkDotNet=v0.13.2.2052-nightly, OS=Windows 10 (10.0.19045.3086)
AMD Ryzen 5 2600, 1 CPU, 12 logical and 6 physical cores
.NET SDK=8.0.100-preview.5.23303.2
[Host] : .NET 8.0.0 (8.0.23.28008), X64 RyuJIT AVX2
Job-XYPMDP : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT AVX2
Job-FNPGHD : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT AVX2
PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:EnableUnsafeBinaryFormatterSerialization=true IterationTime=250.0000 ms
MaxIterationCount=20 MinIterationCount=15 WarmupCount=1
| Method | Job | Toolchain | typeToConvert | Mean | Error | StdDev | Median | Min | Max | Ratio | RatioSD | Allocated | Alloc Ratio |
|------------- |----------- |------------------------------------------------------------------------------------------------------------------------- |--------------------- |---------:|--------:|--------:|---------:|---------:|---------:|------:|--------:|----------:|------------:|
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassIDerived | 146.5 ns | 0.86 ns | 0.72 ns | 146.3 ns | 145.6 ns | 147.9 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassIDerived | 112.6 ns | 0.30 ns | 0.28 ns | 112.6 ns | 112.2 ns | 113.1 ns | 0.77 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassWithNoConverter | 147.3 ns | 0.45 ns | 0.43 ns | 147.3 ns | 146.5 ns | 148.1 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassWithNoConverter | 111.0 ns | 0.17 ns | 0.15 ns | 111.0 ns | 110.7 ns | 111.4 ns | 0.75 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | DerivedClass | 200.3 ns | 1.90 ns | 1.78 ns | 200.4 ns | 196.9 ns | 203.4 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | DerivedClass | 161.5 ns | 5.24 ns | 5.82 ns | 160.1 ns | 155.6 ns | 177.6 ns | 0.81 | 0.03 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | IDerived | 138.1 ns | 0.27 ns | 0.25 ns | 138.0 ns | 137.8 ns | 138.5 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | IDerived | 102.1 ns | 0.08 ns | 0.07 ns | 102.1 ns | 102.1 ns | 102.3 ns | 0.74 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeEnum | 225.1 ns | 0.80 ns | 0.74 ns | 225.3 ns | 223.8 ns | 226.1 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeEnum | 185.4 ns | 1.26 ns | 0.98 ns | 185.2 ns | 184.4 ns | 188.0 ns | 0.82 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Enum | 197.6 ns | 1.10 ns | 0.97 ns | 197.4 ns | 196.2 ns | 199.9 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Enum | 163.6 ns | 4.30 ns | 4.78 ns | 164.2 ns | 155.8 ns | 170.7 ns | 0.83 | 0.02 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Guid | 203.0 ns | 3.68 ns | 3.44 ns | 204.4 ns | 195.4 ns | 209.0 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Guid | 158.9 ns | 3.07 ns | 2.87 ns | 158.0 ns | 155.8 ns | 166.4 ns | 0.78 | 0.02 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32 | 199.0 ns | 3.99 ns | 4.60 ns | 200.4 ns | 191.5 ns | 205.8 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32 | 156.9 ns | 2.71 ns | 2.40 ns | 156.8 ns | 153.7 ns | 162.3 ns | 0.79 | 0.02 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeValueType? | 197.0 ns | 0.80 ns | 0.74 ns | 197.2 ns | 195.3 ns | 197.8 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeValueType? | 161.3 ns | 3.74 ns | 4.31 ns | 160.7 ns | 154.8 ns | 168.4 ns | 0.82 | 0.02 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32? | 207.1 ns | 5.69 ns | 6.32 ns | 206.5 ns | 196.9 ns | 220.8 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32? | 160.0 ns | 2.75 ns | 2.57 ns | 159.9 ns | 156.3 ns | 165.0 ns | 0.77 | 0.02 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | String | 144.4 ns | 0.44 ns | 0.39 ns | 144.3 ns | 143.7 ns | 145.2 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | String | 105.9 ns | 0.09 ns | 0.08 ns | 105.9 ns | 105.8 ns | 106.0 ns | 0.73 | 0.00 | - | NA |

that is, performance of TypeDescriptor.GetConverter has degraded by about 20%
@steveharter
steveharter
commented
Jun 28, 2023

Copy link


Copy Markdown

Contributor
that is, performance of TypeDescriptor.GetConverter has degraded by about 20%
That seems reasonable to me.
@Maximys
Maximys
commented
Jun 30, 2023

Copy link


Copy Markdown

ContributorAuthor
@stephentoub , also lets look to one more branch of my repository: 30024-problem-with-concurrency-typeDescriptor-getconverter(NEW). It contains one more little optimization which provide next result:
// * Summary *
BenchmarkDotNet=v0.13.2.2052-nightly, OS=Windows 10 (10.0.19045.3155)
AMD Ryzen 5 2600, 1 CPU, 12 logical and 6 physical cores
.NET SDK=8.0.100-preview.5.23303.2
[Host] : .NET 8.0.0 (8.0.23.28008), X64 RyuJIT AVX2
Job-ICGKTF : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT AVX2
Job-LAEOHV : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT AVX2
PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:EnableUnsafeBinaryFormatterSerialization=true IterationTime=250.0000 ms
MaxIterationCount=20 MinIterationCount=15 WarmupCount=1
| Method | Job | Toolchain | typeToConvert | Mean | Error | StdDev | Median | Min | Max | Ratio | RatioSD | Allocated | Alloc Ratio |
|------------- |----------- |------------------------------------------------------------------------------------------------------------------------- |--------------------- |---------:|--------:|--------:|---------:|---------:|---------:|------:|--------:|----------:|------------:|
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassIDerived | 123.7 ns | 0.41 ns | 0.34 ns | 123.6 ns | 123.4 ns | 124.6 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassIDerived | 109.3 ns | 0.08 ns | 0.06 ns | 109.4 ns | 109.2 ns | 109.5 ns | 0.88 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassWithNoConverter | 125.5 ns | 0.33 ns | 0.31 ns | 125.5 ns | 124.8 ns | 125.9 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassWithNoConverter | 112.4 ns | 0.24 ns | 0.21 ns | 112.4 ns | 111.8 ns | 112.6 ns | 0.90 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | DerivedClass | 184.7 ns | 1.69 ns | 1.50 ns | 184.2 ns | 182.6 ns | 187.5 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | DerivedClass | 161.5 ns | 1.68 ns | 1.57 ns | 160.9 ns | 159.4 ns | 164.5 ns | 0.88 | 0.01 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | IDerived | 119.7 ns | 0.51 ns | 0.47 ns | 119.7 ns | 118.8 ns | 120.4 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | IDerived | 102.3 ns | 0.07 ns | 0.06 ns | 102.3 ns | 102.1 ns | 102.4 ns | 0.85 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeEnum | 204.4 ns | 0.52 ns | 0.48 ns | 204.4 ns | 203.3 ns | 205.0 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeEnum | 183.8 ns | 0.20 ns | 0.17 ns | 183.8 ns | 183.3 ns | 184.0 ns | 0.90 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Enum | 183.8 ns | 1.75 ns | 1.64 ns | 184.4 ns | 180.3 ns | 186.0 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Enum | 162.6 ns | 2.01 ns | 1.88 ns | 162.3 ns | 159.9 ns | 166.3 ns | 0.88 | 0.01 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Guid | 185.3 ns | 0.85 ns | 0.79 ns | 185.5 ns | 183.2 ns | 186.1 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Guid | 162.5 ns | 0.90 ns | 0.85 ns | 162.7 ns | 159.9 ns | 163.4 ns | 0.88 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32 | 181.4 ns | 1.72 ns | 1.61 ns | 181.8 ns | 178.5 ns | 183.6 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32 | 161.1 ns | 1.30 ns | 1.22 ns | 160.9 ns | 158.7 ns | 162.7 ns | 0.89 | 0.01 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeValueType? | 179.8 ns | 2.63 ns | 2.46 ns | 180.2 ns | 175.7 ns | 183.4 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeValueType? | 160.5 ns | 2.12 ns | 1.98 ns | 160.3 ns | 157.5 ns | 163.9 ns | 0.89 | 0.02 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32? | 179.4 ns | 2.73 ns | 2.55 ns | 179.5 ns | 175.8 ns | 184.2 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32? | 157.7 ns | 1.77 ns | 1.65 ns | 157.1 ns | 155.4 ns | 160.5 ns | 0.88 | 0.01 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | String | 124.0 ns | 0.49 ns | 0.46 ns | 124.2 ns | 123.5 ns | 124.6 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | String | 109.4 ns | 0.06 ns | 0.05 ns | 109.4 ns | 109.3 ns | 109.4 ns | 0.88 | 0.00 | - | NA |

As you can see, perfomance degradation has decreased to 10% after commit 92a2d245958ce41e05398580c0ea45419e05effe. May be it's ok? What do you think?
@steveharter
steveharter
requested a review
from buyaa-nJuly 5, 2023 17:19
@Maximys
Maximysforce-pushed
the
bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter
branch
from
92254ef to
1f20655CompareAugust 4, 2023 04:25
steveharter
steveharter
reviewed
Aug 4, 2023
View reviewed changes
Comment threadsrc/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs
Outdated

Comment threadsrc/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs
Outdated

Comment threadsrc/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs
Comment threadsrc/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs
@steveharter
steveharter
self-requested a review
August 7, 2023 15:11
@steveharter
steveharter
commented
Aug 7, 2023

Copy link


Copy Markdown

Contributor
@stephentoub - PTAL again thanks
@steveharter
steveharter
commented
Aug 9, 2023

Copy link


Copy Markdown

Contributor
Closing this PR for v8 due to risk + schedule; ideally should be done early in the 9.0 branch. There is risk in this PR and we need more time to verify this than what we have left in v8 -- RC1 is almost done.
@Maximys thanks for this PR.
cc @JeremyKuhne - thoughts on fixing this? This issue also occurs in .NET Framework to my knowledge.
@ghostghost
locked as resolved and limited conversation to collaborators
Sep 8, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubAwaiting requested review from stephentoub
@buyaa-nbuyaa-nAwaiting requested review from buyaa-n

1 more reviewer

@steveharterstevehartersteveharter approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.ComponentModelcommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Concurrency issue in TypeDescriptor.GetConverter(type)


4 participants

@Maximys@stephentoub@steveharter@buyaa-n

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Bugfix/30024 problem with concurrency type descriptor getconverter - #85156
Closed
Maximys wants to merge 7 commits into
dotnet:mainfrom 
Maximys:bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter
Closed
Bugfix/30024 problem with concurrency type descriptor getconverter#85156
Maximys wants to merge 7 commits into
dotnet:mainfrom 
Maximys:bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter
Conversation
@Maximys
@MaximysMaximys
commented
Apr 21, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
This PR fix#30024 .

Problem was linked with race condition inside private method System.ComponentModel.TypeDescriptor.CheckDefaultProvider(Type).

I think, it'll better to change base type of WeakHashtable from Hashtable to ConcurrentDictionary<TKey,TValue> and use method GetOrAdd(TKey, Func<TKey,TValue>), but this changes can be very dangerous and I'm afraid to do them.
@ghostghost
added

area-System.ComponentModel

community-contribution
Indicates that the PR has been added by a community member
labels
Apr 21, 2023
@ghost
ghost
commented
Apr 21, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-componentmodel

See info in area-owners.md if you want to be subscribed.
Issue Details
This PR fix #30024 .

Problem was linked with race condition inside private method System.ComponentModel.TypeDescriptor.CheckDefaultProvider(Type).

I think, it'll better to change base type of WeakHashtable from Hashtable to ConcurrentDictionary<TKey,TValue>, but this changes can be very dangerous and I'm afraid to do them.
Author:Maximys
Assignees:-
Labels:
area-System.ComponentModel
Milestone:-
@Maximys
Maximysforce-pushed
the
bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter
branch
from
f3e9d88 to
4693f40CompareApril 21, 2023 11:05

This was referenced May 2, 2023

Closed


Closed

@Maximys
Maximysforce-pushed
the
bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter
branch
from
0f46857 to
e9e137eCompareMay 5, 2023 06:48
stephentoub
stephentoub
reviewed
May 6, 2023
View reviewed changes
Comment threadsrc/libraries/System.ComponentModel.TypeConverter/tests/TypeDescriptionProviderTests.cs
Outdated

stephentoub
stephentoub
requested changes
May 6, 2023
View reviewed changes
@stephentoubstephentoub
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

At quick glance, this doesn't look like a valid change.
The NodeFor method calls CheckDefaultProvider which takes the lock on s_internalSyncObject, and AddProvider calls NodeFor while holding a lock on s_providerTable. That means while holding s_providerTable the implementation can then take a lock on s_internalSyncObject. This change, though, is extending the lock in CheckDefaultProvider such that while holding the lock on s_internalSyncObject, it calls AddProvider, which will try to take a lock on s_providerTable. That means two threads could now deadlock: one holds the lock on s_providerTable and tries to acquire the lock on s_internalSyncObject, and the other holds the lock on s_internalSyncObject and tries to acquire the lock on s_providerTable.
stephentoub

This comment was marked as duplicate.

Sign in to view
@ghostghost
added
the 
needs-author-action
An issue or pull request that requires more info or actions from the author.
label
May 6, 2023
@Maximys
Maximysforce-pushed
the
bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter
branch
from
e9e137e to
92254efCompareMay 13, 2023 16:48
@ghostghost
removed
the 
needs-author-action
An issue or pull request that requires more info or actions from the author.
label
May 13, 2023
@Maximys
Maximys
commented
May 14, 2023

Copy link


Copy Markdown

ContributorAuthor
@stephentoub , thanks for your comment and attention. I had fixed this problem by ReaderWriterLockSlim and made amend of my commits.
@Maximys
Maximys
commented
May 22, 2023

Copy link


Copy Markdown

ContributorAuthor
@stephentoub ?
steveharter
steveharter
approved these changes
Jun 26, 2023
View reviewed changes
@stevehartersteveharter
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

The latest rebase\changes look like they address the feedback from @stephentoub.
@stephentoub
stephentoub
commented
Jun 26, 2023

Copy link


Copy Markdown

Member
The latest rebase\changes look like they address the feedback from @stephentoub.
I've not re-reviewed to determine whether there might still be the potential for deadlocks.
Also, what's the perf impact? It's now taking a reader lock on a path that wasn't previously locked.
@Maximys
Maximys
commented
Jun 27, 2023

Copy link


Copy Markdown

ContributorAuthor
The latest rebase\changes look like they address the feedback from @stephentoub.
I've not re-reviewed to determine whether there might still be the potential for deadlocks.
Also, what's the perf impact? It's now taking a reader lock on a path that wasn't previously locked.
@stephentoub , could you provide some info about steps for perfomance measurement for TypeDescriptor inside current repository? I usually prefer to use BenchmarkDotNet inside console application, but I don't know how to apply this way for profiling code inside .NET Runtime.
@steveharter
steveharter
commented
Jun 27, 2023

Copy link


Copy Markdown

Contributor
@stephentoub , could you provide some info about steps for perfomance measurement for TypeDescriptor inside current repository? I usually prefer to use BenchmarkDotNet inside console application, but I don't know how to apply this way for profiling code inside .NET Runtime.
We do have official benchmarks, but even those call public APIs. Would using variants of your tests with BenchmarkDotNet measure the extra read lock? If the test is very fast, you may want an inner loop within the benchmark method.
@Maximys
Maximys
commented
Jun 28, 2023

Copy link


Copy Markdown

ContributorAuthor
@stephentoub , could you provide some info about steps for perfomance measurement for TypeDescriptor inside current repository? I usually prefer to use BenchmarkDotNet inside console application, but I don't know how to apply this way for profiling code inside .NET Runtime.
We do have official benchmarks, but even those call public APIs. Would using variants of your tests with BenchmarkDotNet measure the extra read lock? If the test is very fast, you may want an inner loop within the benchmark method.
I had create special branch bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter(NEW), where I trying to add required tests. After that, I had build Runtime by
build.cmd clr+libs+libs.tests -rc release -lc release

as it's described here. And then I had trying to use
dotnet run -c Release -f net8.0 --filter "FullyQualifiedName=System.ComponentModel.Tests.PerformanceTests.GetConverter_ByMultithread_New" \ --corerun C:\runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe

command for run one of my tests, but output of this command is:
C:\Program Files\dotnet
C:\Program Files\dotnet\sdk\8.0.100-preview.5.23303.2\Sdks\Microsoft.NET.Sdk\targets\Microsoft.PackageDependencyResolution.targets(266,5): error NETSDK1005: Assets file 'C:\runtime\artifacts\obj\tasks\project.assets.json' doesn't have a target for 'net8.0'. Ensure that restore has run and that you have included 'net8.0' in the TargetFrameworks for your project.

And I can't figure out how to fix this problem. May be somebody could explain me steps for running BenchmarkDotNet inside current repository, as I asked earlier.
@Maximys
Maximys
commented
Jun 28, 2023

Copy link


Copy Markdown

ContributorAuthor
If I run already existed perfomance tests from dotnet/performance by
dotnet run -c Release -f net8.0 --filter "System.ComponentModel.Tests.Perf_TypeDescriptorTests.GetConverter" \ --corerun "C:\runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe" "C:\runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe"
command, then I receive next result:
// * Summary *
BenchmarkDotNet=v0.13.2.2052-nightly, OS=Windows 10 (10.0.19045.3086)
AMD Ryzen 5 2600, 1 CPU, 12 logical and 6 physical cores
.NET SDK=8.0.100-preview.5.23303.2
[Host] : .NET 8.0.0 (8.0.23.28008), X64 RyuJIT AVX2
Job-XYPMDP : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT AVX2
Job-FNPGHD : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT AVX2
PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:EnableUnsafeBinaryFormatterSerialization=true IterationTime=250.0000 ms
MaxIterationCount=20 MinIterationCount=15 WarmupCount=1
| Method | Job | Toolchain | typeToConvert | Mean | Error | StdDev | Median | Min | Max | Ratio | RatioSD | Allocated | Alloc Ratio |
|------------- |----------- |------------------------------------------------------------------------------------------------------------------------- |--------------------- |---------:|--------:|--------:|---------:|---------:|---------:|------:|--------:|----------:|------------:|
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassIDerived | 146.5 ns | 0.86 ns | 0.72 ns | 146.3 ns | 145.6 ns | 147.9 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassIDerived | 112.6 ns | 0.30 ns | 0.28 ns | 112.6 ns | 112.2 ns | 113.1 ns | 0.77 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassWithNoConverter | 147.3 ns | 0.45 ns | 0.43 ns | 147.3 ns | 146.5 ns | 148.1 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassWithNoConverter | 111.0 ns | 0.17 ns | 0.15 ns | 111.0 ns | 110.7 ns | 111.4 ns | 0.75 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | DerivedClass | 200.3 ns | 1.90 ns | 1.78 ns | 200.4 ns | 196.9 ns | 203.4 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | DerivedClass | 161.5 ns | 5.24 ns | 5.82 ns | 160.1 ns | 155.6 ns | 177.6 ns | 0.81 | 0.03 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | IDerived | 138.1 ns | 0.27 ns | 0.25 ns | 138.0 ns | 137.8 ns | 138.5 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | IDerived | 102.1 ns | 0.08 ns | 0.07 ns | 102.1 ns | 102.1 ns | 102.3 ns | 0.74 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeEnum | 225.1 ns | 0.80 ns | 0.74 ns | 225.3 ns | 223.8 ns | 226.1 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeEnum | 185.4 ns | 1.26 ns | 0.98 ns | 185.2 ns | 184.4 ns | 188.0 ns | 0.82 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Enum | 197.6 ns | 1.10 ns | 0.97 ns | 197.4 ns | 196.2 ns | 199.9 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Enum | 163.6 ns | 4.30 ns | 4.78 ns | 164.2 ns | 155.8 ns | 170.7 ns | 0.83 | 0.02 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Guid | 203.0 ns | 3.68 ns | 3.44 ns | 204.4 ns | 195.4 ns | 209.0 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Guid | 158.9 ns | 3.07 ns | 2.87 ns | 158.0 ns | 155.8 ns | 166.4 ns | 0.78 | 0.02 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32 | 199.0 ns | 3.99 ns | 4.60 ns | 200.4 ns | 191.5 ns | 205.8 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32 | 156.9 ns | 2.71 ns | 2.40 ns | 156.8 ns | 153.7 ns | 162.3 ns | 0.79 | 0.02 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeValueType? | 197.0 ns | 0.80 ns | 0.74 ns | 197.2 ns | 195.3 ns | 197.8 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeValueType? | 161.3 ns | 3.74 ns | 4.31 ns | 160.7 ns | 154.8 ns | 168.4 ns | 0.82 | 0.02 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32? | 207.1 ns | 5.69 ns | 6.32 ns | 206.5 ns | 196.9 ns | 220.8 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32? | 160.0 ns | 2.75 ns | 2.57 ns | 159.9 ns | 156.3 ns | 165.0 ns | 0.77 | 0.02 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | String | 144.4 ns | 0.44 ns | 0.39 ns | 144.3 ns | 143.7 ns | 145.2 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | String | 105.9 ns | 0.09 ns | 0.08 ns | 105.9 ns | 105.8 ns | 106.0 ns | 0.73 | 0.00 | - | NA |

that is, performance of TypeDescriptor.GetConverter has degraded by about 20%
@steveharter
steveharter
commented
Jun 28, 2023

Copy link


Copy Markdown

Contributor
that is, performance of TypeDescriptor.GetConverter has degraded by about 20%
That seems reasonable to me.
@Maximys
Maximys
commented
Jun 30, 2023

Copy link


Copy Markdown

ContributorAuthor
@stephentoub , also lets look to one more branch of my repository: 30024-problem-with-concurrency-typeDescriptor-getconverter(NEW). It contains one more little optimization which provide next result:
// * Summary *
BenchmarkDotNet=v0.13.2.2052-nightly, OS=Windows 10 (10.0.19045.3155)
AMD Ryzen 5 2600, 1 CPU, 12 logical and 6 physical cores
.NET SDK=8.0.100-preview.5.23303.2
[Host] : .NET 8.0.0 (8.0.23.28008), X64 RyuJIT AVX2
Job-ICGKTF : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT AVX2
Job-LAEOHV : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT AVX2
PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:EnableUnsafeBinaryFormatterSerialization=true IterationTime=250.0000 ms
MaxIterationCount=20 MinIterationCount=15 WarmupCount=1
| Method | Job | Toolchain | typeToConvert | Mean | Error | StdDev | Median | Min | Max | Ratio | RatioSD | Allocated | Alloc Ratio |
|------------- |----------- |------------------------------------------------------------------------------------------------------------------------- |--------------------- |---------:|--------:|--------:|---------:|---------:|---------:|------:|--------:|----------:|------------:|
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassIDerived | 123.7 ns | 0.41 ns | 0.34 ns | 123.6 ns | 123.4 ns | 124.6 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassIDerived | 109.3 ns | 0.08 ns | 0.06 ns | 109.4 ns | 109.2 ns | 109.5 ns | 0.88 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassWithNoConverter | 125.5 ns | 0.33 ns | 0.31 ns | 125.5 ns | 124.8 ns | 125.9 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassWithNoConverter | 112.4 ns | 0.24 ns | 0.21 ns | 112.4 ns | 111.8 ns | 112.6 ns | 0.90 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | DerivedClass | 184.7 ns | 1.69 ns | 1.50 ns | 184.2 ns | 182.6 ns | 187.5 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | DerivedClass | 161.5 ns | 1.68 ns | 1.57 ns | 160.9 ns | 159.4 ns | 164.5 ns | 0.88 | 0.01 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | IDerived | 119.7 ns | 0.51 ns | 0.47 ns | 119.7 ns | 118.8 ns | 120.4 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | IDerived | 102.3 ns | 0.07 ns | 0.06 ns | 102.3 ns | 102.1 ns | 102.4 ns | 0.85 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeEnum | 204.4 ns | 0.52 ns | 0.48 ns | 204.4 ns | 203.3 ns | 205.0 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeEnum | 183.8 ns | 0.20 ns | 0.17 ns | 183.8 ns | 183.3 ns | 184.0 ns | 0.90 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Enum | 183.8 ns | 1.75 ns | 1.64 ns | 184.4 ns | 180.3 ns | 186.0 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Enum | 162.6 ns | 2.01 ns | 1.88 ns | 162.3 ns | 159.9 ns | 166.3 ns | 0.88 | 0.01 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Guid | 185.3 ns | 0.85 ns | 0.79 ns | 185.5 ns | 183.2 ns | 186.1 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Guid | 162.5 ns | 0.90 ns | 0.85 ns | 162.7 ns | 159.9 ns | 163.4 ns | 0.88 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32 | 181.4 ns | 1.72 ns | 1.61 ns | 181.8 ns | 178.5 ns | 183.6 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32 | 161.1 ns | 1.30 ns | 1.22 ns | 160.9 ns | 158.7 ns | 162.7 ns | 0.89 | 0.01 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeValueType? | 179.8 ns | 2.63 ns | 2.46 ns | 180.2 ns | 175.7 ns | 183.4 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeValueType? | 160.5 ns | 2.12 ns | 1.98 ns | 160.3 ns | 157.5 ns | 163.9 ns | 0.89 | 0.02 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32? | 179.4 ns | 2.73 ns | 2.55 ns | 179.5 ns | 175.8 ns | 184.2 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32? | 157.7 ns | 1.77 ns | 1.65 ns | 157.1 ns | 155.4 ns | 160.5 ns | 0.88 | 0.01 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | String | 124.0 ns | 0.49 ns | 0.46 ns | 124.2 ns | 123.5 ns | 124.6 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | String | 109.4 ns | 0.06 ns | 0.05 ns | 109.4 ns | 109.3 ns | 109.4 ns | 0.88 | 0.00 | - | NA |

As you can see, perfomance degradation has decreased to 10% after commit 92a2d245958ce41e05398580c0ea45419e05effe. May be it's ok? What do you think?
@steveharter
steveharter
requested a review
from buyaa-nJuly 5, 2023 17:19
@Maximys
Maximysforce-pushed
the
bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter
branch
from
92254ef to
1f20655CompareAugust 4, 2023 04:25
steveharter
steveharter
reviewed
Aug 4, 2023
View reviewed changes
Comment threadsrc/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs
Outdated

Comment threadsrc/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs
Outdated

Comment threadsrc/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs
Comment threadsrc/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs
@steveharter
steveharter
self-requested a review
August 7, 2023 15:11
@steveharter
steveharter
commented
Aug 7, 2023

Copy link


Copy Markdown

Contributor
@stephentoub - PTAL again thanks
@steveharter
steveharter
commented
Aug 9, 2023

Copy link


Copy Markdown

Contributor
Closing this PR for v8 due to risk + schedule; ideally should be done early in the 9.0 branch. There is risk in this PR and we need more time to verify this than what we have left in v8 -- RC1 is almost done.
@Maximys thanks for this PR.
cc @JeremyKuhne - thoughts on fixing this? This issue also occurs in .NET Framework to my knowledge.
@ghostghost
locked as resolved and limited conversation to collaborators
Sep 8, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubAwaiting requested review from stephentoub
@buyaa-nbuyaa-nAwaiting requested review from buyaa-n

1 more reviewer

@steveharterstevehartersteveharter approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.ComponentModelcommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Concurrency issue in TypeDescriptor.GetConverter(type)


4 participants

@Maximys@stephentoub@steveharter@buyaa-n

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Bugfix/30024 problem with concurrency type descriptor getconverter - #85156
Closed
Maximys wants to merge 7 commits into
dotnet:mainfrom 
Maximys:bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter
Closed
Bugfix/30024 problem with concurrency type descriptor getconverter#85156
Maximys wants to merge 7 commits into
dotnet:mainfrom 
Maximys:bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter
Conversation
@Maximys
@MaximysMaximys
commented
Apr 21, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
This PR fix#30024 .

Problem was linked with race condition inside private method System.ComponentModel.TypeDescriptor.CheckDefaultProvider(Type).

I think, it'll better to change base type of WeakHashtable from Hashtable to ConcurrentDictionary<TKey,TValue> and use method GetOrAdd(TKey, Func<TKey,TValue>), but this changes can be very dangerous and I'm afraid to do them.
@ghostghost
added

area-System.ComponentModel

community-contribution
Indicates that the PR has been added by a community member
labels
Apr 21, 2023
@ghost
ghost
commented
Apr 21, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-componentmodel

See info in area-owners.md if you want to be subscribed.
Issue Details
This PR fix #30024 .

Problem was linked with race condition inside private method System.ComponentModel.TypeDescriptor.CheckDefaultProvider(Type).

I think, it'll better to change base type of WeakHashtable from Hashtable to ConcurrentDictionary<TKey,TValue>, but this changes can be very dangerous and I'm afraid to do them.
Author:Maximys
Assignees:-
Labels:
area-System.ComponentModel
Milestone:-
@Maximys
Maximysforce-pushed
the
bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter
branch
from
f3e9d88 to
4693f40CompareApril 21, 2023 11:05

This was referenced May 2, 2023

Closed


Closed

@Maximys
Maximysforce-pushed
the
bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter
branch
from
0f46857 to
e9e137eCompareMay 5, 2023 06:48
stephentoub
stephentoub
reviewed
May 6, 2023
View reviewed changes
Comment threadsrc/libraries/System.ComponentModel.TypeConverter/tests/TypeDescriptionProviderTests.cs
Outdated

stephentoub
stephentoub
requested changes
May 6, 2023
View reviewed changes
@stephentoubstephentoub
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

At quick glance, this doesn't look like a valid change.
The NodeFor method calls CheckDefaultProvider which takes the lock on s_internalSyncObject, and AddProvider calls NodeFor while holding a lock on s_providerTable. That means while holding s_providerTable the implementation can then take a lock on s_internalSyncObject. This change, though, is extending the lock in CheckDefaultProvider such that while holding the lock on s_internalSyncObject, it calls AddProvider, which will try to take a lock on s_providerTable. That means two threads could now deadlock: one holds the lock on s_providerTable and tries to acquire the lock on s_internalSyncObject, and the other holds the lock on s_internalSyncObject and tries to acquire the lock on s_providerTable.
stephentoub

This comment was marked as duplicate.

Sign in to view
@ghostghost
added
the 
needs-author-action
An issue or pull request that requires more info or actions from the author.
label
May 6, 2023
@Maximys
Maximysforce-pushed
the
bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter
branch
from
e9e137e to
92254efCompareMay 13, 2023 16:48
@ghostghost
removed
the 
needs-author-action
An issue or pull request that requires more info or actions from the author.
label
May 13, 2023
@Maximys
Maximys
commented
May 14, 2023

Copy link


Copy Markdown

ContributorAuthor
@stephentoub , thanks for your comment and attention. I had fixed this problem by ReaderWriterLockSlim and made amend of my commits.
@Maximys
Maximys
commented
May 22, 2023

Copy link


Copy Markdown

ContributorAuthor
@stephentoub ?
steveharter
steveharter
approved these changes
Jun 26, 2023
View reviewed changes
@stevehartersteveharter
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

The latest rebase\changes look like they address the feedback from @stephentoub.
@stephentoub
stephentoub
commented
Jun 26, 2023

Copy link


Copy Markdown

Member
The latest rebase\changes look like they address the feedback from @stephentoub.
I've not re-reviewed to determine whether there might still be the potential for deadlocks.
Also, what's the perf impact? It's now taking a reader lock on a path that wasn't previously locked.
@Maximys
Maximys
commented
Jun 27, 2023

Copy link


Copy Markdown

ContributorAuthor
The latest rebase\changes look like they address the feedback from @stephentoub.
I've not re-reviewed to determine whether there might still be the potential for deadlocks.
Also, what's the perf impact? It's now taking a reader lock on a path that wasn't previously locked.
@stephentoub , could you provide some info about steps for perfomance measurement for TypeDescriptor inside current repository? I usually prefer to use BenchmarkDotNet inside console application, but I don't know how to apply this way for profiling code inside .NET Runtime.
@steveharter
steveharter
commented
Jun 27, 2023

Copy link


Copy Markdown

Contributor
@stephentoub , could you provide some info about steps for perfomance measurement for TypeDescriptor inside current repository? I usually prefer to use BenchmarkDotNet inside console application, but I don't know how to apply this way for profiling code inside .NET Runtime.
We do have official benchmarks, but even those call public APIs. Would using variants of your tests with BenchmarkDotNet measure the extra read lock? If the test is very fast, you may want an inner loop within the benchmark method.
@Maximys
Maximys
commented
Jun 28, 2023

Copy link


Copy Markdown

ContributorAuthor
@stephentoub , could you provide some info about steps for perfomance measurement for TypeDescriptor inside current repository? I usually prefer to use BenchmarkDotNet inside console application, but I don't know how to apply this way for profiling code inside .NET Runtime.
We do have official benchmarks, but even those call public APIs. Would using variants of your tests with BenchmarkDotNet measure the extra read lock? If the test is very fast, you may want an inner loop within the benchmark method.
I had create special branch bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter(NEW), where I trying to add required tests. After that, I had build Runtime by
build.cmd clr+libs+libs.tests -rc release -lc release

as it's described here. And then I had trying to use
dotnet run -c Release -f net8.0 --filter "FullyQualifiedName=System.ComponentModel.Tests.PerformanceTests.GetConverter_ByMultithread_New" \ --corerun C:\runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe

command for run one of my tests, but output of this command is:
C:\Program Files\dotnet
C:\Program Files\dotnet\sdk\8.0.100-preview.5.23303.2\Sdks\Microsoft.NET.Sdk\targets\Microsoft.PackageDependencyResolution.targets(266,5): error NETSDK1005: Assets file 'C:\runtime\artifacts\obj\tasks\project.assets.json' doesn't have a target for 'net8.0'. Ensure that restore has run and that you have included 'net8.0' in the TargetFrameworks for your project.

And I can't figure out how to fix this problem. May be somebody could explain me steps for running BenchmarkDotNet inside current repository, as I asked earlier.
@Maximys
Maximys
commented
Jun 28, 2023

Copy link


Copy Markdown

ContributorAuthor
If I run already existed perfomance tests from dotnet/performance by
dotnet run -c Release -f net8.0 --filter "System.ComponentModel.Tests.Perf_TypeDescriptorTests.GetConverter" \ --corerun "C:\runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe" "C:\runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe"
command, then I receive next result:
// * Summary *
BenchmarkDotNet=v0.13.2.2052-nightly, OS=Windows 10 (10.0.19045.3086)
AMD Ryzen 5 2600, 1 CPU, 12 logical and 6 physical cores
.NET SDK=8.0.100-preview.5.23303.2
[Host] : .NET 8.0.0 (8.0.23.28008), X64 RyuJIT AVX2
Job-XYPMDP : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT AVX2
Job-FNPGHD : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT AVX2
PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:EnableUnsafeBinaryFormatterSerialization=true IterationTime=250.0000 ms
MaxIterationCount=20 MinIterationCount=15 WarmupCount=1
| Method | Job | Toolchain | typeToConvert | Mean | Error | StdDev | Median | Min | Max | Ratio | RatioSD | Allocated | Alloc Ratio |
|------------- |----------- |------------------------------------------------------------------------------------------------------------------------- |--------------------- |---------:|--------:|--------:|---------:|---------:|---------:|------:|--------:|----------:|------------:|
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassIDerived | 146.5 ns | 0.86 ns | 0.72 ns | 146.3 ns | 145.6 ns | 147.9 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassIDerived | 112.6 ns | 0.30 ns | 0.28 ns | 112.6 ns | 112.2 ns | 113.1 ns | 0.77 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassWithNoConverter | 147.3 ns | 0.45 ns | 0.43 ns | 147.3 ns | 146.5 ns | 148.1 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassWithNoConverter | 111.0 ns | 0.17 ns | 0.15 ns | 111.0 ns | 110.7 ns | 111.4 ns | 0.75 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | DerivedClass | 200.3 ns | 1.90 ns | 1.78 ns | 200.4 ns | 196.9 ns | 203.4 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | DerivedClass | 161.5 ns | 5.24 ns | 5.82 ns | 160.1 ns | 155.6 ns | 177.6 ns | 0.81 | 0.03 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | IDerived | 138.1 ns | 0.27 ns | 0.25 ns | 138.0 ns | 137.8 ns | 138.5 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | IDerived | 102.1 ns | 0.08 ns | 0.07 ns | 102.1 ns | 102.1 ns | 102.3 ns | 0.74 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeEnum | 225.1 ns | 0.80 ns | 0.74 ns | 225.3 ns | 223.8 ns | 226.1 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeEnum | 185.4 ns | 1.26 ns | 0.98 ns | 185.2 ns | 184.4 ns | 188.0 ns | 0.82 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Enum | 197.6 ns | 1.10 ns | 0.97 ns | 197.4 ns | 196.2 ns | 199.9 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Enum | 163.6 ns | 4.30 ns | 4.78 ns | 164.2 ns | 155.8 ns | 170.7 ns | 0.83 | 0.02 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Guid | 203.0 ns | 3.68 ns | 3.44 ns | 204.4 ns | 195.4 ns | 209.0 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Guid | 158.9 ns | 3.07 ns | 2.87 ns | 158.0 ns | 155.8 ns | 166.4 ns | 0.78 | 0.02 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32 | 199.0 ns | 3.99 ns | 4.60 ns | 200.4 ns | 191.5 ns | 205.8 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32 | 156.9 ns | 2.71 ns | 2.40 ns | 156.8 ns | 153.7 ns | 162.3 ns | 0.79 | 0.02 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeValueType? | 197.0 ns | 0.80 ns | 0.74 ns | 197.2 ns | 195.3 ns | 197.8 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeValueType? | 161.3 ns | 3.74 ns | 4.31 ns | 160.7 ns | 154.8 ns | 168.4 ns | 0.82 | 0.02 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32? | 207.1 ns | 5.69 ns | 6.32 ns | 206.5 ns | 196.9 ns | 220.8 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32? | 160.0 ns | 2.75 ns | 2.57 ns | 159.9 ns | 156.3 ns | 165.0 ns | 0.77 | 0.02 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | String | 144.4 ns | 0.44 ns | 0.39 ns | 144.3 ns | 143.7 ns | 145.2 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | String | 105.9 ns | 0.09 ns | 0.08 ns | 105.9 ns | 105.8 ns | 106.0 ns | 0.73 | 0.00 | - | NA |

that is, performance of TypeDescriptor.GetConverter has degraded by about 20%
@steveharter
steveharter
commented
Jun 28, 2023

Copy link


Copy Markdown

Contributor
that is, performance of TypeDescriptor.GetConverter has degraded by about 20%
That seems reasonable to me.
@Maximys
Maximys
commented
Jun 30, 2023

Copy link


Copy Markdown

ContributorAuthor
@stephentoub , also lets look to one more branch of my repository: 30024-problem-with-concurrency-typeDescriptor-getconverter(NEW). It contains one more little optimization which provide next result:
// * Summary *
BenchmarkDotNet=v0.13.2.2052-nightly, OS=Windows 10 (10.0.19045.3155)
AMD Ryzen 5 2600, 1 CPU, 12 logical and 6 physical cores
.NET SDK=8.0.100-preview.5.23303.2
[Host] : .NET 8.0.0 (8.0.23.28008), X64 RyuJIT AVX2
Job-ICGKTF : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT AVX2
Job-LAEOHV : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT AVX2
PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:EnableUnsafeBinaryFormatterSerialization=true IterationTime=250.0000 ms
MaxIterationCount=20 MinIterationCount=15 WarmupCount=1
| Method | Job | Toolchain | typeToConvert | Mean | Error | StdDev | Median | Min | Max | Ratio | RatioSD | Allocated | Alloc Ratio |
|------------- |----------- |------------------------------------------------------------------------------------------------------------------------- |--------------------- |---------:|--------:|--------:|---------:|---------:|---------:|------:|--------:|----------:|------------:|
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassIDerived | 123.7 ns | 0.41 ns | 0.34 ns | 123.6 ns | 123.4 ns | 124.6 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassIDerived | 109.3 ns | 0.08 ns | 0.06 ns | 109.4 ns | 109.2 ns | 109.5 ns | 0.88 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassWithNoConverter | 125.5 ns | 0.33 ns | 0.31 ns | 125.5 ns | 124.8 ns | 125.9 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassWithNoConverter | 112.4 ns | 0.24 ns | 0.21 ns | 112.4 ns | 111.8 ns | 112.6 ns | 0.90 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | DerivedClass | 184.7 ns | 1.69 ns | 1.50 ns | 184.2 ns | 182.6 ns | 187.5 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | DerivedClass | 161.5 ns | 1.68 ns | 1.57 ns | 160.9 ns | 159.4 ns | 164.5 ns | 0.88 | 0.01 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | IDerived | 119.7 ns | 0.51 ns | 0.47 ns | 119.7 ns | 118.8 ns | 120.4 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | IDerived | 102.3 ns | 0.07 ns | 0.06 ns | 102.3 ns | 102.1 ns | 102.4 ns | 0.85 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeEnum | 204.4 ns | 0.52 ns | 0.48 ns | 204.4 ns | 203.3 ns | 205.0 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeEnum | 183.8 ns | 0.20 ns | 0.17 ns | 183.8 ns | 183.3 ns | 184.0 ns | 0.90 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Enum | 183.8 ns | 1.75 ns | 1.64 ns | 184.4 ns | 180.3 ns | 186.0 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Enum | 162.6 ns | 2.01 ns | 1.88 ns | 162.3 ns | 159.9 ns | 166.3 ns | 0.88 | 0.01 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Guid | 185.3 ns | 0.85 ns | 0.79 ns | 185.5 ns | 183.2 ns | 186.1 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Guid | 162.5 ns | 0.90 ns | 0.85 ns | 162.7 ns | 159.9 ns | 163.4 ns | 0.88 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32 | 181.4 ns | 1.72 ns | 1.61 ns | 181.8 ns | 178.5 ns | 183.6 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32 | 161.1 ns | 1.30 ns | 1.22 ns | 160.9 ns | 158.7 ns | 162.7 ns | 0.89 | 0.01 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeValueType? | 179.8 ns | 2.63 ns | 2.46 ns | 180.2 ns | 175.7 ns | 183.4 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeValueType? | 160.5 ns | 2.12 ns | 1.98 ns | 160.3 ns | 157.5 ns | 163.9 ns | 0.89 | 0.02 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32? | 179.4 ns | 2.73 ns | 2.55 ns | 179.5 ns | 175.8 ns | 184.2 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32? | 157.7 ns | 1.77 ns | 1.65 ns | 157.1 ns | 155.4 ns | 160.5 ns | 0.88 | 0.01 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | String | 124.0 ns | 0.49 ns | 0.46 ns | 124.2 ns | 123.5 ns | 124.6 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | String | 109.4 ns | 0.06 ns | 0.05 ns | 109.4 ns | 109.3 ns | 109.4 ns | 0.88 | 0.00 | - | NA |

As you can see, perfomance degradation has decreased to 10% after commit 92a2d245958ce41e05398580c0ea45419e05effe. May be it's ok? What do you think?
@steveharter
steveharter
requested a review
from buyaa-nJuly 5, 2023 17:19
@Maximys
Maximysforce-pushed
the
bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter
branch
from
92254ef to
1f20655CompareAugust 4, 2023 04:25
steveharter
steveharter
reviewed
Aug 4, 2023
View reviewed changes
Comment threadsrc/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs
Outdated

Comment threadsrc/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs
Outdated

Comment threadsrc/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs
Comment threadsrc/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs
@steveharter
steveharter
self-requested a review
August 7, 2023 15:11
@steveharter
steveharter
commented
Aug 7, 2023

Copy link


Copy Markdown

Contributor
@stephentoub - PTAL again thanks
@steveharter
steveharter
commented
Aug 9, 2023

Copy link


Copy Markdown

Contributor
Closing this PR for v8 due to risk + schedule; ideally should be done early in the 9.0 branch. There is risk in this PR and we need more time to verify this than what we have left in v8 -- RC1 is almost done.
@Maximys thanks for this PR.
cc @JeremyKuhne - thoughts on fixing this? This issue also occurs in .NET Framework to my knowledge.
@ghostghost
locked as resolved and limited conversation to collaborators
Sep 8, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubAwaiting requested review from stephentoub
@buyaa-nbuyaa-nAwaiting requested review from buyaa-n

1 more reviewer

@steveharterstevehartersteveharter approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.ComponentModelcommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Concurrency issue in TypeDescriptor.GetConverter(type)


4 participants

@Maximys@stephentoub@steveharter@buyaa-n

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Bugfix/30024 problem with concurrency type descriptor getconverter - #85156
Closed
Maximys wants to merge 7 commits into
dotnet:mainfrom 
Maximys:bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter
Closed
Bugfix/30024 problem with concurrency type descriptor getconverter#85156
Maximys wants to merge 7 commits into
dotnet:mainfrom 
Maximys:bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter
Conversation
@Maximys
@MaximysMaximys
commented
Apr 21, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
This PR fix#30024 .

Problem was linked with race condition inside private method System.ComponentModel.TypeDescriptor.CheckDefaultProvider(Type).

I think, it'll better to change base type of WeakHashtable from Hashtable to ConcurrentDictionary<TKey,TValue> and use method GetOrAdd(TKey, Func<TKey,TValue>), but this changes can be very dangerous and I'm afraid to do them.
@ghostghost
added

area-System.ComponentModel

community-contribution
Indicates that the PR has been added by a community member
labels
Apr 21, 2023
@ghost
ghost
commented
Apr 21, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-componentmodel

See info in area-owners.md if you want to be subscribed.
Issue Details
This PR fix #30024 .

Problem was linked with race condition inside private method System.ComponentModel.TypeDescriptor.CheckDefaultProvider(Type).

I think, it'll better to change base type of WeakHashtable from Hashtable to ConcurrentDictionary<TKey,TValue>, but this changes can be very dangerous and I'm afraid to do them.
Author:Maximys
Assignees:-
Labels:
area-System.ComponentModel
Milestone:-
@Maximys
Maximysforce-pushed
the
bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter
branch
from
f3e9d88 to
4693f40CompareApril 21, 2023 11:05

This was referenced May 2, 2023

Closed


Closed

@Maximys
Maximysforce-pushed
the
bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter
branch
from
0f46857 to
e9e137eCompareMay 5, 2023 06:48
stephentoub
stephentoub
reviewed
May 6, 2023
View reviewed changes
Comment threadsrc/libraries/System.ComponentModel.TypeConverter/tests/TypeDescriptionProviderTests.cs
Outdated

stephentoub
stephentoub
requested changes
May 6, 2023
View reviewed changes
@stephentoubstephentoub
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

At quick glance, this doesn't look like a valid change.
The NodeFor method calls CheckDefaultProvider which takes the lock on s_internalSyncObject, and AddProvider calls NodeFor while holding a lock on s_providerTable. That means while holding s_providerTable the implementation can then take a lock on s_internalSyncObject. This change, though, is extending the lock in CheckDefaultProvider such that while holding the lock on s_internalSyncObject, it calls AddProvider, which will try to take a lock on s_providerTable. That means two threads could now deadlock: one holds the lock on s_providerTable and tries to acquire the lock on s_internalSyncObject, and the other holds the lock on s_internalSyncObject and tries to acquire the lock on s_providerTable.
stephentoub

This comment was marked as duplicate.

Sign in to view
@ghostghost
added
the 
needs-author-action
An issue or pull request that requires more info or actions from the author.
label
May 6, 2023
@Maximys
Maximysforce-pushed
the
bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter
branch
from
e9e137e to
92254efCompareMay 13, 2023 16:48
@ghostghost
removed
the 
needs-author-action
An issue or pull request that requires more info or actions from the author.
label
May 13, 2023
@Maximys
Maximys
commented
May 14, 2023

Copy link


Copy Markdown

ContributorAuthor
@stephentoub , thanks for your comment and attention. I had fixed this problem by ReaderWriterLockSlim and made amend of my commits.
@Maximys
Maximys
commented
May 22, 2023

Copy link


Copy Markdown

ContributorAuthor
@stephentoub ?
steveharter
steveharter
approved these changes
Jun 26, 2023
View reviewed changes
@stevehartersteveharter
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

The latest rebase\changes look like they address the feedback from @stephentoub.
@stephentoub
stephentoub
commented
Jun 26, 2023

Copy link


Copy Markdown

Member
The latest rebase\changes look like they address the feedback from @stephentoub.
I've not re-reviewed to determine whether there might still be the potential for deadlocks.
Also, what's the perf impact? It's now taking a reader lock on a path that wasn't previously locked.
@Maximys
Maximys
commented
Jun 27, 2023

Copy link


Copy Markdown

ContributorAuthor
The latest rebase\changes look like they address the feedback from @stephentoub.
I've not re-reviewed to determine whether there might still be the potential for deadlocks.
Also, what's the perf impact? It's now taking a reader lock on a path that wasn't previously locked.
@stephentoub , could you provide some info about steps for perfomance measurement for TypeDescriptor inside current repository? I usually prefer to use BenchmarkDotNet inside console application, but I don't know how to apply this way for profiling code inside .NET Runtime.
@steveharter
steveharter
commented
Jun 27, 2023

Copy link


Copy Markdown

Contributor
@stephentoub , could you provide some info about steps for perfomance measurement for TypeDescriptor inside current repository? I usually prefer to use BenchmarkDotNet inside console application, but I don't know how to apply this way for profiling code inside .NET Runtime.
We do have official benchmarks, but even those call public APIs. Would using variants of your tests with BenchmarkDotNet measure the extra read lock? If the test is very fast, you may want an inner loop within the benchmark method.
@Maximys
Maximys
commented
Jun 28, 2023

Copy link


Copy Markdown

ContributorAuthor
@stephentoub , could you provide some info about steps for perfomance measurement for TypeDescriptor inside current repository? I usually prefer to use BenchmarkDotNet inside console application, but I don't know how to apply this way for profiling code inside .NET Runtime.
We do have official benchmarks, but even those call public APIs. Would using variants of your tests with BenchmarkDotNet measure the extra read lock? If the test is very fast, you may want an inner loop within the benchmark method.
I had create special branch bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter(NEW), where I trying to add required tests. After that, I had build Runtime by
build.cmd clr+libs+libs.tests -rc release -lc release

as it's described here. And then I had trying to use
dotnet run -c Release -f net8.0 --filter "FullyQualifiedName=System.ComponentModel.Tests.PerformanceTests.GetConverter_ByMultithread_New" \ --corerun C:\runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe

command for run one of my tests, but output of this command is:
C:\Program Files\dotnet
C:\Program Files\dotnet\sdk\8.0.100-preview.5.23303.2\Sdks\Microsoft.NET.Sdk\targets\Microsoft.PackageDependencyResolution.targets(266,5): error NETSDK1005: Assets file 'C:\runtime\artifacts\obj\tasks\project.assets.json' doesn't have a target for 'net8.0'. Ensure that restore has run and that you have included 'net8.0' in the TargetFrameworks for your project.

And I can't figure out how to fix this problem. May be somebody could explain me steps for running BenchmarkDotNet inside current repository, as I asked earlier.
@Maximys
Maximys
commented
Jun 28, 2023

Copy link


Copy Markdown

ContributorAuthor
If I run already existed perfomance tests from dotnet/performance by
dotnet run -c Release -f net8.0 --filter "System.ComponentModel.Tests.Perf_TypeDescriptorTests.GetConverter" \ --corerun "C:\runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe" "C:\runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe"
command, then I receive next result:
// * Summary *
BenchmarkDotNet=v0.13.2.2052-nightly, OS=Windows 10 (10.0.19045.3086)
AMD Ryzen 5 2600, 1 CPU, 12 logical and 6 physical cores
.NET SDK=8.0.100-preview.5.23303.2
[Host] : .NET 8.0.0 (8.0.23.28008), X64 RyuJIT AVX2
Job-XYPMDP : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT AVX2
Job-FNPGHD : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT AVX2
PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:EnableUnsafeBinaryFormatterSerialization=true IterationTime=250.0000 ms
MaxIterationCount=20 MinIterationCount=15 WarmupCount=1
| Method | Job | Toolchain | typeToConvert | Mean | Error | StdDev | Median | Min | Max | Ratio | RatioSD | Allocated | Alloc Ratio |
|------------- |----------- |------------------------------------------------------------------------------------------------------------------------- |--------------------- |---------:|--------:|--------:|---------:|---------:|---------:|------:|--------:|----------:|------------:|
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassIDerived | 146.5 ns | 0.86 ns | 0.72 ns | 146.3 ns | 145.6 ns | 147.9 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassIDerived | 112.6 ns | 0.30 ns | 0.28 ns | 112.6 ns | 112.2 ns | 113.1 ns | 0.77 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassWithNoConverter | 147.3 ns | 0.45 ns | 0.43 ns | 147.3 ns | 146.5 ns | 148.1 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassWithNoConverter | 111.0 ns | 0.17 ns | 0.15 ns | 111.0 ns | 110.7 ns | 111.4 ns | 0.75 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | DerivedClass | 200.3 ns | 1.90 ns | 1.78 ns | 200.4 ns | 196.9 ns | 203.4 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | DerivedClass | 161.5 ns | 5.24 ns | 5.82 ns | 160.1 ns | 155.6 ns | 177.6 ns | 0.81 | 0.03 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | IDerived | 138.1 ns | 0.27 ns | 0.25 ns | 138.0 ns | 137.8 ns | 138.5 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | IDerived | 102.1 ns | 0.08 ns | 0.07 ns | 102.1 ns | 102.1 ns | 102.3 ns | 0.74 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeEnum | 225.1 ns | 0.80 ns | 0.74 ns | 225.3 ns | 223.8 ns | 226.1 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeEnum | 185.4 ns | 1.26 ns | 0.98 ns | 185.2 ns | 184.4 ns | 188.0 ns | 0.82 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Enum | 197.6 ns | 1.10 ns | 0.97 ns | 197.4 ns | 196.2 ns | 199.9 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Enum | 163.6 ns | 4.30 ns | 4.78 ns | 164.2 ns | 155.8 ns | 170.7 ns | 0.83 | 0.02 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Guid | 203.0 ns | 3.68 ns | 3.44 ns | 204.4 ns | 195.4 ns | 209.0 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Guid | 158.9 ns | 3.07 ns | 2.87 ns | 158.0 ns | 155.8 ns | 166.4 ns | 0.78 | 0.02 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32 | 199.0 ns | 3.99 ns | 4.60 ns | 200.4 ns | 191.5 ns | 205.8 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32 | 156.9 ns | 2.71 ns | 2.40 ns | 156.8 ns | 153.7 ns | 162.3 ns | 0.79 | 0.02 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeValueType? | 197.0 ns | 0.80 ns | 0.74 ns | 197.2 ns | 195.3 ns | 197.8 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeValueType? | 161.3 ns | 3.74 ns | 4.31 ns | 160.7 ns | 154.8 ns | 168.4 ns | 0.82 | 0.02 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32? | 207.1 ns | 5.69 ns | 6.32 ns | 206.5 ns | 196.9 ns | 220.8 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32? | 160.0 ns | 2.75 ns | 2.57 ns | 159.9 ns | 156.3 ns | 165.0 ns | 0.77 | 0.02 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | String | 144.4 ns | 0.44 ns | 0.39 ns | 144.3 ns | 143.7 ns | 145.2 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | String | 105.9 ns | 0.09 ns | 0.08 ns | 105.9 ns | 105.8 ns | 106.0 ns | 0.73 | 0.00 | - | NA |

that is, performance of TypeDescriptor.GetConverter has degraded by about 20%
@steveharter
steveharter
commented
Jun 28, 2023

Copy link


Copy Markdown

Contributor
that is, performance of TypeDescriptor.GetConverter has degraded by about 20%
That seems reasonable to me.
@Maximys
Maximys
commented
Jun 30, 2023

Copy link


Copy Markdown

ContributorAuthor
@stephentoub , also lets look to one more branch of my repository: 30024-problem-with-concurrency-typeDescriptor-getconverter(NEW). It contains one more little optimization which provide next result:
// * Summary *
BenchmarkDotNet=v0.13.2.2052-nightly, OS=Windows 10 (10.0.19045.3155)
AMD Ryzen 5 2600, 1 CPU, 12 logical and 6 physical cores
.NET SDK=8.0.100-preview.5.23303.2
[Host] : .NET 8.0.0 (8.0.23.28008), X64 RyuJIT AVX2
Job-ICGKTF : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT AVX2
Job-LAEOHV : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT AVX2
PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:EnableUnsafeBinaryFormatterSerialization=true IterationTime=250.0000 ms
MaxIterationCount=20 MinIterationCount=15 WarmupCount=1
| Method | Job | Toolchain | typeToConvert | Mean | Error | StdDev | Median | Min | Max | Ratio | RatioSD | Allocated | Alloc Ratio |
|------------- |----------- |------------------------------------------------------------------------------------------------------------------------- |--------------------- |---------:|--------:|--------:|---------:|---------:|---------:|------:|--------:|----------:|------------:|
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassIDerived | 123.7 ns | 0.41 ns | 0.34 ns | 123.6 ns | 123.4 ns | 124.6 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassIDerived | 109.3 ns | 0.08 ns | 0.06 ns | 109.4 ns | 109.2 ns | 109.5 ns | 0.88 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassWithNoConverter | 125.5 ns | 0.33 ns | 0.31 ns | 125.5 ns | 124.8 ns | 125.9 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassWithNoConverter | 112.4 ns | 0.24 ns | 0.21 ns | 112.4 ns | 111.8 ns | 112.6 ns | 0.90 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | DerivedClass | 184.7 ns | 1.69 ns | 1.50 ns | 184.2 ns | 182.6 ns | 187.5 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | DerivedClass | 161.5 ns | 1.68 ns | 1.57 ns | 160.9 ns | 159.4 ns | 164.5 ns | 0.88 | 0.01 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | IDerived | 119.7 ns | 0.51 ns | 0.47 ns | 119.7 ns | 118.8 ns | 120.4 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | IDerived | 102.3 ns | 0.07 ns | 0.06 ns | 102.3 ns | 102.1 ns | 102.4 ns | 0.85 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeEnum | 204.4 ns | 0.52 ns | 0.48 ns | 204.4 ns | 203.3 ns | 205.0 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeEnum | 183.8 ns | 0.20 ns | 0.17 ns | 183.8 ns | 183.3 ns | 184.0 ns | 0.90 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Enum | 183.8 ns | 1.75 ns | 1.64 ns | 184.4 ns | 180.3 ns | 186.0 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Enum | 162.6 ns | 2.01 ns | 1.88 ns | 162.3 ns | 159.9 ns | 166.3 ns | 0.88 | 0.01 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Guid | 185.3 ns | 0.85 ns | 0.79 ns | 185.5 ns | 183.2 ns | 186.1 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Guid | 162.5 ns | 0.90 ns | 0.85 ns | 162.7 ns | 159.9 ns | 163.4 ns | 0.88 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32 | 181.4 ns | 1.72 ns | 1.61 ns | 181.8 ns | 178.5 ns | 183.6 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32 | 161.1 ns | 1.30 ns | 1.22 ns | 160.9 ns | 158.7 ns | 162.7 ns | 0.89 | 0.01 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeValueType? | 179.8 ns | 2.63 ns | 2.46 ns | 180.2 ns | 175.7 ns | 183.4 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeValueType? | 160.5 ns | 2.12 ns | 1.98 ns | 160.3 ns | 157.5 ns | 163.9 ns | 0.89 | 0.02 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32? | 179.4 ns | 2.73 ns | 2.55 ns | 179.5 ns | 175.8 ns | 184.2 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32? | 157.7 ns | 1.77 ns | 1.65 ns | 157.1 ns | 155.4 ns | 160.5 ns | 0.88 | 0.01 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | String | 124.0 ns | 0.49 ns | 0.46 ns | 124.2 ns | 123.5 ns | 124.6 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | String | 109.4 ns | 0.06 ns | 0.05 ns | 109.4 ns | 109.3 ns | 109.4 ns | 0.88 | 0.00 | - | NA |

As you can see, perfomance degradation has decreased to 10% after commit 92a2d245958ce41e05398580c0ea45419e05effe. May be it's ok? What do you think?
@steveharter
steveharter
requested a review
from buyaa-nJuly 5, 2023 17:19
@Maximys
Maximysforce-pushed
the
bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter
branch
from
92254ef to
1f20655CompareAugust 4, 2023 04:25
steveharter
steveharter
reviewed
Aug 4, 2023
View reviewed changes
Comment threadsrc/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs
Outdated

Comment threadsrc/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs
Outdated

Comment threadsrc/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs
Comment threadsrc/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs
@steveharter
steveharter
self-requested a review
August 7, 2023 15:11
@steveharter
steveharter
commented
Aug 7, 2023

Copy link


Copy Markdown

Contributor
@stephentoub - PTAL again thanks
@steveharter
steveharter
commented
Aug 9, 2023

Copy link


Copy Markdown

Contributor
Closing this PR for v8 due to risk + schedule; ideally should be done early in the 9.0 branch. There is risk in this PR and we need more time to verify this than what we have left in v8 -- RC1 is almost done.
@Maximys thanks for this PR.
cc @JeremyKuhne - thoughts on fixing this? This issue also occurs in .NET Framework to my knowledge.
@ghostghost
locked as resolved and limited conversation to collaborators
Sep 8, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubAwaiting requested review from stephentoub
@buyaa-nbuyaa-nAwaiting requested review from buyaa-n

1 more reviewer

@steveharterstevehartersteveharter approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.ComponentModelcommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Concurrency issue in TypeDescriptor.GetConverter(type)


4 participants

@Maximys@stephentoub@steveharter@buyaa-n

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Bugfix/30024 problem with concurrency type descriptor getconverter - #85156
Closed
Maximys wants to merge 7 commits into
dotnet:mainfrom 
Maximys:bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter
Closed
Bugfix/30024 problem with concurrency type descriptor getconverter#85156
Maximys wants to merge 7 commits into
dotnet:mainfrom 
Maximys:bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter
Conversation
@Maximys
@MaximysMaximys
commented
Apr 21, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
This PR fix#30024 .

Problem was linked with race condition inside private method System.ComponentModel.TypeDescriptor.CheckDefaultProvider(Type).

I think, it'll better to change base type of WeakHashtable from Hashtable to ConcurrentDictionary<TKey,TValue> and use method GetOrAdd(TKey, Func<TKey,TValue>), but this changes can be very dangerous and I'm afraid to do them.
@ghostghost
added

area-System.ComponentModel

community-contribution
Indicates that the PR has been added by a community member
labels
Apr 21, 2023
@ghost
ghost
commented
Apr 21, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-componentmodel

See info in area-owners.md if you want to be subscribed.
Issue Details
This PR fix #30024 .

Problem was linked with race condition inside private method System.ComponentModel.TypeDescriptor.CheckDefaultProvider(Type).

I think, it'll better to change base type of WeakHashtable from Hashtable to ConcurrentDictionary<TKey,TValue>, but this changes can be very dangerous and I'm afraid to do them.
Author:Maximys
Assignees:-
Labels:
area-System.ComponentModel
Milestone:-
@Maximys
Maximysforce-pushed
the
bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter
branch
from
f3e9d88 to
4693f40CompareApril 21, 2023 11:05

This was referenced May 2, 2023

Closed


Closed

@Maximys
Maximysforce-pushed
the
bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter
branch
from
0f46857 to
e9e137eCompareMay 5, 2023 06:48
stephentoub
stephentoub
reviewed
May 6, 2023
View reviewed changes
Comment threadsrc/libraries/System.ComponentModel.TypeConverter/tests/TypeDescriptionProviderTests.cs
Outdated

stephentoub
stephentoub
requested changes
May 6, 2023
View reviewed changes
@stephentoubstephentoub
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

At quick glance, this doesn't look like a valid change.
The NodeFor method calls CheckDefaultProvider which takes the lock on s_internalSyncObject, and AddProvider calls NodeFor while holding a lock on s_providerTable. That means while holding s_providerTable the implementation can then take a lock on s_internalSyncObject. This change, though, is extending the lock in CheckDefaultProvider such that while holding the lock on s_internalSyncObject, it calls AddProvider, which will try to take a lock on s_providerTable. That means two threads could now deadlock: one holds the lock on s_providerTable and tries to acquire the lock on s_internalSyncObject, and the other holds the lock on s_internalSyncObject and tries to acquire the lock on s_providerTable.
stephentoub

This comment was marked as duplicate.

Sign in to view
@ghostghost
added
the 
needs-author-action
An issue or pull request that requires more info or actions from the author.
label
May 6, 2023
@Maximys
Maximysforce-pushed
the
bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter
branch
from
e9e137e to
92254efCompareMay 13, 2023 16:48
@ghostghost
removed
the 
needs-author-action
An issue or pull request that requires more info or actions from the author.
label
May 13, 2023
@Maximys
Maximys
commented
May 14, 2023

Copy link


Copy Markdown

ContributorAuthor
@stephentoub , thanks for your comment and attention. I had fixed this problem by ReaderWriterLockSlim and made amend of my commits.
@Maximys
Maximys
commented
May 22, 2023

Copy link


Copy Markdown

ContributorAuthor
@stephentoub ?
steveharter
steveharter
approved these changes
Jun 26, 2023
View reviewed changes
@stevehartersteveharter
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

The latest rebase\changes look like they address the feedback from @stephentoub.
@stephentoub
stephentoub
commented
Jun 26, 2023

Copy link


Copy Markdown

Member
The latest rebase\changes look like they address the feedback from @stephentoub.
I've not re-reviewed to determine whether there might still be the potential for deadlocks.
Also, what's the perf impact? It's now taking a reader lock on a path that wasn't previously locked.
@Maximys
Maximys
commented
Jun 27, 2023

Copy link


Copy Markdown

ContributorAuthor
The latest rebase\changes look like they address the feedback from @stephentoub.
I've not re-reviewed to determine whether there might still be the potential for deadlocks.
Also, what's the perf impact? It's now taking a reader lock on a path that wasn't previously locked.
@stephentoub , could you provide some info about steps for perfomance measurement for TypeDescriptor inside current repository? I usually prefer to use BenchmarkDotNet inside console application, but I don't know how to apply this way for profiling code inside .NET Runtime.
@steveharter
steveharter
commented
Jun 27, 2023

Copy link


Copy Markdown

Contributor
@stephentoub , could you provide some info about steps for perfomance measurement for TypeDescriptor inside current repository? I usually prefer to use BenchmarkDotNet inside console application, but I don't know how to apply this way for profiling code inside .NET Runtime.
We do have official benchmarks, but even those call public APIs. Would using variants of your tests with BenchmarkDotNet measure the extra read lock? If the test is very fast, you may want an inner loop within the benchmark method.
@Maximys
Maximys
commented
Jun 28, 2023

Copy link


Copy Markdown

ContributorAuthor
@stephentoub , could you provide some info about steps for perfomance measurement for TypeDescriptor inside current repository? I usually prefer to use BenchmarkDotNet inside console application, but I don't know how to apply this way for profiling code inside .NET Runtime.
We do have official benchmarks, but even those call public APIs. Would using variants of your tests with BenchmarkDotNet measure the extra read lock? If the test is very fast, you may want an inner loop within the benchmark method.
I had create special branch bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter(NEW), where I trying to add required tests. After that, I had build Runtime by
build.cmd clr+libs+libs.tests -rc release -lc release

as it's described here. And then I had trying to use
dotnet run -c Release -f net8.0 --filter "FullyQualifiedName=System.ComponentModel.Tests.PerformanceTests.GetConverter_ByMultithread_New" \ --corerun C:\runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe

command for run one of my tests, but output of this command is:
C:\Program Files\dotnet
C:\Program Files\dotnet\sdk\8.0.100-preview.5.23303.2\Sdks\Microsoft.NET.Sdk\targets\Microsoft.PackageDependencyResolution.targets(266,5): error NETSDK1005: Assets file 'C:\runtime\artifacts\obj\tasks\project.assets.json' doesn't have a target for 'net8.0'. Ensure that restore has run and that you have included 'net8.0' in the TargetFrameworks for your project.

And I can't figure out how to fix this problem. May be somebody could explain me steps for running BenchmarkDotNet inside current repository, as I asked earlier.
@Maximys
Maximys
commented
Jun 28, 2023

Copy link


Copy Markdown

ContributorAuthor
If I run already existed perfomance tests from dotnet/performance by
dotnet run -c Release -f net8.0 --filter "System.ComponentModel.Tests.Perf_TypeDescriptorTests.GetConverter" \ --corerun "C:\runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe" "C:\runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe"
command, then I receive next result:
// * Summary *
BenchmarkDotNet=v0.13.2.2052-nightly, OS=Windows 10 (10.0.19045.3086)
AMD Ryzen 5 2600, 1 CPU, 12 logical and 6 physical cores
.NET SDK=8.0.100-preview.5.23303.2
[Host] : .NET 8.0.0 (8.0.23.28008), X64 RyuJIT AVX2
Job-XYPMDP : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT AVX2
Job-FNPGHD : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT AVX2
PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:EnableUnsafeBinaryFormatterSerialization=true IterationTime=250.0000 ms
MaxIterationCount=20 MinIterationCount=15 WarmupCount=1
| Method | Job | Toolchain | typeToConvert | Mean | Error | StdDev | Median | Min | Max | Ratio | RatioSD | Allocated | Alloc Ratio |
|------------- |----------- |------------------------------------------------------------------------------------------------------------------------- |--------------------- |---------:|--------:|--------:|---------:|---------:|---------:|------:|--------:|----------:|------------:|
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassIDerived | 146.5 ns | 0.86 ns | 0.72 ns | 146.3 ns | 145.6 ns | 147.9 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassIDerived | 112.6 ns | 0.30 ns | 0.28 ns | 112.6 ns | 112.2 ns | 113.1 ns | 0.77 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassWithNoConverter | 147.3 ns | 0.45 ns | 0.43 ns | 147.3 ns | 146.5 ns | 148.1 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassWithNoConverter | 111.0 ns | 0.17 ns | 0.15 ns | 111.0 ns | 110.7 ns | 111.4 ns | 0.75 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | DerivedClass | 200.3 ns | 1.90 ns | 1.78 ns | 200.4 ns | 196.9 ns | 203.4 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | DerivedClass | 161.5 ns | 5.24 ns | 5.82 ns | 160.1 ns | 155.6 ns | 177.6 ns | 0.81 | 0.03 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | IDerived | 138.1 ns | 0.27 ns | 0.25 ns | 138.0 ns | 137.8 ns | 138.5 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | IDerived | 102.1 ns | 0.08 ns | 0.07 ns | 102.1 ns | 102.1 ns | 102.3 ns | 0.74 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeEnum | 225.1 ns | 0.80 ns | 0.74 ns | 225.3 ns | 223.8 ns | 226.1 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeEnum | 185.4 ns | 1.26 ns | 0.98 ns | 185.2 ns | 184.4 ns | 188.0 ns | 0.82 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Enum | 197.6 ns | 1.10 ns | 0.97 ns | 197.4 ns | 196.2 ns | 199.9 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Enum | 163.6 ns | 4.30 ns | 4.78 ns | 164.2 ns | 155.8 ns | 170.7 ns | 0.83 | 0.02 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Guid | 203.0 ns | 3.68 ns | 3.44 ns | 204.4 ns | 195.4 ns | 209.0 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Guid | 158.9 ns | 3.07 ns | 2.87 ns | 158.0 ns | 155.8 ns | 166.4 ns | 0.78 | 0.02 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32 | 199.0 ns | 3.99 ns | 4.60 ns | 200.4 ns | 191.5 ns | 205.8 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32 | 156.9 ns | 2.71 ns | 2.40 ns | 156.8 ns | 153.7 ns | 162.3 ns | 0.79 | 0.02 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeValueType? | 197.0 ns | 0.80 ns | 0.74 ns | 197.2 ns | 195.3 ns | 197.8 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeValueType? | 161.3 ns | 3.74 ns | 4.31 ns | 160.7 ns | 154.8 ns | 168.4 ns | 0.82 | 0.02 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32? | 207.1 ns | 5.69 ns | 6.32 ns | 206.5 ns | 196.9 ns | 220.8 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32? | 160.0 ns | 2.75 ns | 2.57 ns | 159.9 ns | 156.3 ns | 165.0 ns | 0.77 | 0.02 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | String | 144.4 ns | 0.44 ns | 0.39 ns | 144.3 ns | 143.7 ns | 145.2 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | String | 105.9 ns | 0.09 ns | 0.08 ns | 105.9 ns | 105.8 ns | 106.0 ns | 0.73 | 0.00 | - | NA |

that is, performance of TypeDescriptor.GetConverter has degraded by about 20%
@steveharter
steveharter
commented
Jun 28, 2023

Copy link


Copy Markdown

Contributor
that is, performance of TypeDescriptor.GetConverter has degraded by about 20%
That seems reasonable to me.
@Maximys
Maximys
commented
Jun 30, 2023

Copy link


Copy Markdown

ContributorAuthor
@stephentoub , also lets look to one more branch of my repository: 30024-problem-with-concurrency-typeDescriptor-getconverter(NEW). It contains one more little optimization which provide next result:
// * Summary *
BenchmarkDotNet=v0.13.2.2052-nightly, OS=Windows 10 (10.0.19045.3155)
AMD Ryzen 5 2600, 1 CPU, 12 logical and 6 physical cores
.NET SDK=8.0.100-preview.5.23303.2
[Host] : .NET 8.0.0 (8.0.23.28008), X64 RyuJIT AVX2
Job-ICGKTF : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT AVX2
Job-LAEOHV : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT AVX2
PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:EnableUnsafeBinaryFormatterSerialization=true IterationTime=250.0000 ms
MaxIterationCount=20 MinIterationCount=15 WarmupCount=1
| Method | Job | Toolchain | typeToConvert | Mean | Error | StdDev | Median | Min | Max | Ratio | RatioSD | Allocated | Alloc Ratio |
|------------- |----------- |------------------------------------------------------------------------------------------------------------------------- |--------------------- |---------:|--------:|--------:|---------:|---------:|---------:|------:|--------:|----------:|------------:|
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassIDerived | 123.7 ns | 0.41 ns | 0.34 ns | 123.6 ns | 123.4 ns | 124.6 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassIDerived | 109.3 ns | 0.08 ns | 0.06 ns | 109.4 ns | 109.2 ns | 109.5 ns | 0.88 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassWithNoConverter | 125.5 ns | 0.33 ns | 0.31 ns | 125.5 ns | 124.8 ns | 125.9 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassWithNoConverter | 112.4 ns | 0.24 ns | 0.21 ns | 112.4 ns | 111.8 ns | 112.6 ns | 0.90 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | DerivedClass | 184.7 ns | 1.69 ns | 1.50 ns | 184.2 ns | 182.6 ns | 187.5 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | DerivedClass | 161.5 ns | 1.68 ns | 1.57 ns | 160.9 ns | 159.4 ns | 164.5 ns | 0.88 | 0.01 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | IDerived | 119.7 ns | 0.51 ns | 0.47 ns | 119.7 ns | 118.8 ns | 120.4 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | IDerived | 102.3 ns | 0.07 ns | 0.06 ns | 102.3 ns | 102.1 ns | 102.4 ns | 0.85 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeEnum | 204.4 ns | 0.52 ns | 0.48 ns | 204.4 ns | 203.3 ns | 205.0 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeEnum | 183.8 ns | 0.20 ns | 0.17 ns | 183.8 ns | 183.3 ns | 184.0 ns | 0.90 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Enum | 183.8 ns | 1.75 ns | 1.64 ns | 184.4 ns | 180.3 ns | 186.0 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Enum | 162.6 ns | 2.01 ns | 1.88 ns | 162.3 ns | 159.9 ns | 166.3 ns | 0.88 | 0.01 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Guid | 185.3 ns | 0.85 ns | 0.79 ns | 185.5 ns | 183.2 ns | 186.1 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Guid | 162.5 ns | 0.90 ns | 0.85 ns | 162.7 ns | 159.9 ns | 163.4 ns | 0.88 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32 | 181.4 ns | 1.72 ns | 1.61 ns | 181.8 ns | 178.5 ns | 183.6 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32 | 161.1 ns | 1.30 ns | 1.22 ns | 160.9 ns | 158.7 ns | 162.7 ns | 0.89 | 0.01 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeValueType? | 179.8 ns | 2.63 ns | 2.46 ns | 180.2 ns | 175.7 ns | 183.4 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeValueType? | 160.5 ns | 2.12 ns | 1.98 ns | 160.3 ns | 157.5 ns | 163.9 ns | 0.89 | 0.02 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32? | 179.4 ns | 2.73 ns | 2.55 ns | 179.5 ns | 175.8 ns | 184.2 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32? | 157.7 ns | 1.77 ns | 1.65 ns | 157.1 ns | 155.4 ns | 160.5 ns | 0.88 | 0.01 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | String | 124.0 ns | 0.49 ns | 0.46 ns | 124.2 ns | 123.5 ns | 124.6 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | String | 109.4 ns | 0.06 ns | 0.05 ns | 109.4 ns | 109.3 ns | 109.4 ns | 0.88 | 0.00 | - | NA |

As you can see, perfomance degradation has decreased to 10% after commit 92a2d245958ce41e05398580c0ea45419e05effe. May be it's ok? What do you think?
@steveharter
steveharter
requested a review
from buyaa-nJuly 5, 2023 17:19
@Maximys
Maximysforce-pushed
the
bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter
branch
from
92254ef to
1f20655CompareAugust 4, 2023 04:25
steveharter
steveharter
reviewed
Aug 4, 2023
View reviewed changes
Comment threadsrc/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs
Outdated

Comment threadsrc/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs
Outdated

Comment threadsrc/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs
Comment threadsrc/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs
@steveharter
steveharter
self-requested a review
August 7, 2023 15:11
@steveharter
steveharter
commented
Aug 7, 2023

Copy link


Copy Markdown

Contributor
@stephentoub - PTAL again thanks
@steveharter
steveharter
commented
Aug 9, 2023

Copy link


Copy Markdown

Contributor
Closing this PR for v8 due to risk + schedule; ideally should be done early in the 9.0 branch. There is risk in this PR and we need more time to verify this than what we have left in v8 -- RC1 is almost done.
@Maximys thanks for this PR.
cc @JeremyKuhne - thoughts on fixing this? This issue also occurs in .NET Framework to my knowledge.
@ghostghost
locked as resolved and limited conversation to collaborators
Sep 8, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubAwaiting requested review from stephentoub
@buyaa-nbuyaa-nAwaiting requested review from buyaa-n

1 more reviewer

@steveharterstevehartersteveharter approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.ComponentModelcommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Concurrency issue in TypeDescriptor.GetConverter(type)


4 participants

@Maximys@stephentoub@steveharter@buyaa-n

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Bugfix/30024 problem with concurrency type descriptor getconverter - #85156
Closed
Maximys wants to merge 7 commits into
dotnet:mainfrom 
Maximys:bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter
Closed
Bugfix/30024 problem with concurrency type descriptor getconverter#85156
Maximys wants to merge 7 commits into
dotnet:mainfrom 
Maximys:bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter
Conversation
@Maximys
@MaximysMaximys
commented
Apr 21, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
This PR fix#30024 .

Problem was linked with race condition inside private method System.ComponentModel.TypeDescriptor.CheckDefaultProvider(Type).

I think, it'll better to change base type of WeakHashtable from Hashtable to ConcurrentDictionary<TKey,TValue> and use method GetOrAdd(TKey, Func<TKey,TValue>), but this changes can be very dangerous and I'm afraid to do them.
@ghostghost
added

area-System.ComponentModel

community-contribution
Indicates that the PR has been added by a community member
labels
Apr 21, 2023
@ghost
ghost
commented
Apr 21, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-componentmodel

See info in area-owners.md if you want to be subscribed.
Issue Details
This PR fix #30024 .

Problem was linked with race condition inside private method System.ComponentModel.TypeDescriptor.CheckDefaultProvider(Type).

I think, it'll better to change base type of WeakHashtable from Hashtable to ConcurrentDictionary<TKey,TValue>, but this changes can be very dangerous and I'm afraid to do them.
Author:Maximys
Assignees:-
Labels:
area-System.ComponentModel
Milestone:-
@Maximys
Maximysforce-pushed
the
bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter
branch
from
f3e9d88 to
4693f40CompareApril 21, 2023 11:05

This was referenced May 2, 2023

Closed


Closed

@Maximys
Maximysforce-pushed
the
bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter
branch
from
0f46857 to
e9e137eCompareMay 5, 2023 06:48
stephentoub
stephentoub
reviewed
May 6, 2023
View reviewed changes
Comment threadsrc/libraries/System.ComponentModel.TypeConverter/tests/TypeDescriptionProviderTests.cs
Outdated

stephentoub
stephentoub
requested changes
May 6, 2023
View reviewed changes
@stephentoubstephentoub
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

At quick glance, this doesn't look like a valid change.
The NodeFor method calls CheckDefaultProvider which takes the lock on s_internalSyncObject, and AddProvider calls NodeFor while holding a lock on s_providerTable. That means while holding s_providerTable the implementation can then take a lock on s_internalSyncObject. This change, though, is extending the lock in CheckDefaultProvider such that while holding the lock on s_internalSyncObject, it calls AddProvider, which will try to take a lock on s_providerTable. That means two threads could now deadlock: one holds the lock on s_providerTable and tries to acquire the lock on s_internalSyncObject, and the other holds the lock on s_internalSyncObject and tries to acquire the lock on s_providerTable.
stephentoub

This comment was marked as duplicate.

Sign in to view
@ghostghost
added
the 
needs-author-action
An issue or pull request that requires more info or actions from the author.
label
May 6, 2023
@Maximys
Maximysforce-pushed
the
bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter
branch
from
e9e137e to
92254efCompareMay 13, 2023 16:48
@ghostghost
removed
the 
needs-author-action
An issue or pull request that requires more info or actions from the author.
label
May 13, 2023
@Maximys
Maximys
commented
May 14, 2023

Copy link


Copy Markdown

ContributorAuthor
@stephentoub , thanks for your comment and attention. I had fixed this problem by ReaderWriterLockSlim and made amend of my commits.
@Maximys
Maximys
commented
May 22, 2023

Copy link


Copy Markdown

ContributorAuthor
@stephentoub ?
steveharter
steveharter
approved these changes
Jun 26, 2023
View reviewed changes
@stevehartersteveharter
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

The latest rebase\changes look like they address the feedback from @stephentoub.
@stephentoub
stephentoub
commented
Jun 26, 2023

Copy link


Copy Markdown

Member
The latest rebase\changes look like they address the feedback from @stephentoub.
I've not re-reviewed to determine whether there might still be the potential for deadlocks.
Also, what's the perf impact? It's now taking a reader lock on a path that wasn't previously locked.
@Maximys
Maximys
commented
Jun 27, 2023

Copy link


Copy Markdown

ContributorAuthor
The latest rebase\changes look like they address the feedback from @stephentoub.
I've not re-reviewed to determine whether there might still be the potential for deadlocks.
Also, what's the perf impact? It's now taking a reader lock on a path that wasn't previously locked.
@stephentoub , could you provide some info about steps for perfomance measurement for TypeDescriptor inside current repository? I usually prefer to use BenchmarkDotNet inside console application, but I don't know how to apply this way for profiling code inside .NET Runtime.
@steveharter
steveharter
commented
Jun 27, 2023

Copy link


Copy Markdown

Contributor
@stephentoub , could you provide some info about steps for perfomance measurement for TypeDescriptor inside current repository? I usually prefer to use BenchmarkDotNet inside console application, but I don't know how to apply this way for profiling code inside .NET Runtime.
We do have official benchmarks, but even those call public APIs. Would using variants of your tests with BenchmarkDotNet measure the extra read lock? If the test is very fast, you may want an inner loop within the benchmark method.
@Maximys
Maximys
commented
Jun 28, 2023

Copy link


Copy Markdown

ContributorAuthor
@stephentoub , could you provide some info about steps for perfomance measurement for TypeDescriptor inside current repository? I usually prefer to use BenchmarkDotNet inside console application, but I don't know how to apply this way for profiling code inside .NET Runtime.
We do have official benchmarks, but even those call public APIs. Would using variants of your tests with BenchmarkDotNet measure the extra read lock? If the test is very fast, you may want an inner loop within the benchmark method.
I had create special branch bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter(NEW), where I trying to add required tests. After that, I had build Runtime by
build.cmd clr+libs+libs.tests -rc release -lc release

as it's described here. And then I had trying to use
dotnet run -c Release -f net8.0 --filter "FullyQualifiedName=System.ComponentModel.Tests.PerformanceTests.GetConverter_ByMultithread_New" \ --corerun C:\runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe

command for run one of my tests, but output of this command is:
C:\Program Files\dotnet
C:\Program Files\dotnet\sdk\8.0.100-preview.5.23303.2\Sdks\Microsoft.NET.Sdk\targets\Microsoft.PackageDependencyResolution.targets(266,5): error NETSDK1005: Assets file 'C:\runtime\artifacts\obj\tasks\project.assets.json' doesn't have a target for 'net8.0'. Ensure that restore has run and that you have included 'net8.0' in the TargetFrameworks for your project.

And I can't figure out how to fix this problem. May be somebody could explain me steps for running BenchmarkDotNet inside current repository, as I asked earlier.
@Maximys
Maximys
commented
Jun 28, 2023

Copy link


Copy Markdown

ContributorAuthor
If I run already existed perfomance tests from dotnet/performance by
dotnet run -c Release -f net8.0 --filter "System.ComponentModel.Tests.Perf_TypeDescriptorTests.GetConverter" \ --corerun "C:\runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe" "C:\runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe"
command, then I receive next result:
// * Summary *
BenchmarkDotNet=v0.13.2.2052-nightly, OS=Windows 10 (10.0.19045.3086)
AMD Ryzen 5 2600, 1 CPU, 12 logical and 6 physical cores
.NET SDK=8.0.100-preview.5.23303.2
[Host] : .NET 8.0.0 (8.0.23.28008), X64 RyuJIT AVX2
Job-XYPMDP : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT AVX2
Job-FNPGHD : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT AVX2
PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:EnableUnsafeBinaryFormatterSerialization=true IterationTime=250.0000 ms
MaxIterationCount=20 MinIterationCount=15 WarmupCount=1
| Method | Job | Toolchain | typeToConvert | Mean | Error | StdDev | Median | Min | Max | Ratio | RatioSD | Allocated | Alloc Ratio |
|------------- |----------- |------------------------------------------------------------------------------------------------------------------------- |--------------------- |---------:|--------:|--------:|---------:|---------:|---------:|------:|--------:|----------:|------------:|
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassIDerived | 146.5 ns | 0.86 ns | 0.72 ns | 146.3 ns | 145.6 ns | 147.9 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassIDerived | 112.6 ns | 0.30 ns | 0.28 ns | 112.6 ns | 112.2 ns | 113.1 ns | 0.77 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassWithNoConverter | 147.3 ns | 0.45 ns | 0.43 ns | 147.3 ns | 146.5 ns | 148.1 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassWithNoConverter | 111.0 ns | 0.17 ns | 0.15 ns | 111.0 ns | 110.7 ns | 111.4 ns | 0.75 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | DerivedClass | 200.3 ns | 1.90 ns | 1.78 ns | 200.4 ns | 196.9 ns | 203.4 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | DerivedClass | 161.5 ns | 5.24 ns | 5.82 ns | 160.1 ns | 155.6 ns | 177.6 ns | 0.81 | 0.03 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | IDerived | 138.1 ns | 0.27 ns | 0.25 ns | 138.0 ns | 137.8 ns | 138.5 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | IDerived | 102.1 ns | 0.08 ns | 0.07 ns | 102.1 ns | 102.1 ns | 102.3 ns | 0.74 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeEnum | 225.1 ns | 0.80 ns | 0.74 ns | 225.3 ns | 223.8 ns | 226.1 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeEnum | 185.4 ns | 1.26 ns | 0.98 ns | 185.2 ns | 184.4 ns | 188.0 ns | 0.82 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Enum | 197.6 ns | 1.10 ns | 0.97 ns | 197.4 ns | 196.2 ns | 199.9 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Enum | 163.6 ns | 4.30 ns | 4.78 ns | 164.2 ns | 155.8 ns | 170.7 ns | 0.83 | 0.02 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Guid | 203.0 ns | 3.68 ns | 3.44 ns | 204.4 ns | 195.4 ns | 209.0 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Guid | 158.9 ns | 3.07 ns | 2.87 ns | 158.0 ns | 155.8 ns | 166.4 ns | 0.78 | 0.02 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32 | 199.0 ns | 3.99 ns | 4.60 ns | 200.4 ns | 191.5 ns | 205.8 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32 | 156.9 ns | 2.71 ns | 2.40 ns | 156.8 ns | 153.7 ns | 162.3 ns | 0.79 | 0.02 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeValueType? | 197.0 ns | 0.80 ns | 0.74 ns | 197.2 ns | 195.3 ns | 197.8 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeValueType? | 161.3 ns | 3.74 ns | 4.31 ns | 160.7 ns | 154.8 ns | 168.4 ns | 0.82 | 0.02 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32? | 207.1 ns | 5.69 ns | 6.32 ns | 206.5 ns | 196.9 ns | 220.8 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32? | 160.0 ns | 2.75 ns | 2.57 ns | 159.9 ns | 156.3 ns | 165.0 ns | 0.77 | 0.02 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | String | 144.4 ns | 0.44 ns | 0.39 ns | 144.3 ns | 143.7 ns | 145.2 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | String | 105.9 ns | 0.09 ns | 0.08 ns | 105.9 ns | 105.8 ns | 106.0 ns | 0.73 | 0.00 | - | NA |

that is, performance of TypeDescriptor.GetConverter has degraded by about 20%
@steveharter
steveharter
commented
Jun 28, 2023

Copy link


Copy Markdown

Contributor
that is, performance of TypeDescriptor.GetConverter has degraded by about 20%
That seems reasonable to me.
@Maximys
Maximys
commented
Jun 30, 2023

Copy link


Copy Markdown

ContributorAuthor
@stephentoub , also lets look to one more branch of my repository: 30024-problem-with-concurrency-typeDescriptor-getconverter(NEW). It contains one more little optimization which provide next result:
// * Summary *
BenchmarkDotNet=v0.13.2.2052-nightly, OS=Windows 10 (10.0.19045.3155)
AMD Ryzen 5 2600, 1 CPU, 12 logical and 6 physical cores
.NET SDK=8.0.100-preview.5.23303.2
[Host] : .NET 8.0.0 (8.0.23.28008), X64 RyuJIT AVX2
Job-ICGKTF : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT AVX2
Job-LAEOHV : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT AVX2
PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:EnableUnsafeBinaryFormatterSerialization=true IterationTime=250.0000 ms
MaxIterationCount=20 MinIterationCount=15 WarmupCount=1
| Method | Job | Toolchain | typeToConvert | Mean | Error | StdDev | Median | Min | Max | Ratio | RatioSD | Allocated | Alloc Ratio |
|------------- |----------- |------------------------------------------------------------------------------------------------------------------------- |--------------------- |---------:|--------:|--------:|---------:|---------:|---------:|------:|--------:|----------:|------------:|
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassIDerived | 123.7 ns | 0.41 ns | 0.34 ns | 123.6 ns | 123.4 ns | 124.6 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassIDerived | 109.3 ns | 0.08 ns | 0.06 ns | 109.4 ns | 109.2 ns | 109.5 ns | 0.88 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassWithNoConverter | 125.5 ns | 0.33 ns | 0.31 ns | 125.5 ns | 124.8 ns | 125.9 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassWithNoConverter | 112.4 ns | 0.24 ns | 0.21 ns | 112.4 ns | 111.8 ns | 112.6 ns | 0.90 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | DerivedClass | 184.7 ns | 1.69 ns | 1.50 ns | 184.2 ns | 182.6 ns | 187.5 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | DerivedClass | 161.5 ns | 1.68 ns | 1.57 ns | 160.9 ns | 159.4 ns | 164.5 ns | 0.88 | 0.01 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | IDerived | 119.7 ns | 0.51 ns | 0.47 ns | 119.7 ns | 118.8 ns | 120.4 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | IDerived | 102.3 ns | 0.07 ns | 0.06 ns | 102.3 ns | 102.1 ns | 102.4 ns | 0.85 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeEnum | 204.4 ns | 0.52 ns | 0.48 ns | 204.4 ns | 203.3 ns | 205.0 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeEnum | 183.8 ns | 0.20 ns | 0.17 ns | 183.8 ns | 183.3 ns | 184.0 ns | 0.90 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Enum | 183.8 ns | 1.75 ns | 1.64 ns | 184.4 ns | 180.3 ns | 186.0 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Enum | 162.6 ns | 2.01 ns | 1.88 ns | 162.3 ns | 159.9 ns | 166.3 ns | 0.88 | 0.01 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Guid | 185.3 ns | 0.85 ns | 0.79 ns | 185.5 ns | 183.2 ns | 186.1 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Guid | 162.5 ns | 0.90 ns | 0.85 ns | 162.7 ns | 159.9 ns | 163.4 ns | 0.88 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32 | 181.4 ns | 1.72 ns | 1.61 ns | 181.8 ns | 178.5 ns | 183.6 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32 | 161.1 ns | 1.30 ns | 1.22 ns | 160.9 ns | 158.7 ns | 162.7 ns | 0.89 | 0.01 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeValueType? | 179.8 ns | 2.63 ns | 2.46 ns | 180.2 ns | 175.7 ns | 183.4 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeValueType? | 160.5 ns | 2.12 ns | 1.98 ns | 160.3 ns | 157.5 ns | 163.9 ns | 0.89 | 0.02 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32? | 179.4 ns | 2.73 ns | 2.55 ns | 179.5 ns | 175.8 ns | 184.2 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32? | 157.7 ns | 1.77 ns | 1.65 ns | 157.1 ns | 155.4 ns | 160.5 ns | 0.88 | 0.01 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | String | 124.0 ns | 0.49 ns | 0.46 ns | 124.2 ns | 123.5 ns | 124.6 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | String | 109.4 ns | 0.06 ns | 0.05 ns | 109.4 ns | 109.3 ns | 109.4 ns | 0.88 | 0.00 | - | NA |

As you can see, perfomance degradation has decreased to 10% after commit 92a2d245958ce41e05398580c0ea45419e05effe. May be it's ok? What do you think?
@steveharter
steveharter
requested a review
from buyaa-nJuly 5, 2023 17:19
@Maximys
Maximysforce-pushed
the
bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter
branch
from
92254ef to
1f20655CompareAugust 4, 2023 04:25
steveharter
steveharter
reviewed
Aug 4, 2023
View reviewed changes
Comment threadsrc/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs
Outdated

Comment threadsrc/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs
Outdated

Comment threadsrc/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs
Comment threadsrc/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs
@steveharter
steveharter
self-requested a review
August 7, 2023 15:11
@steveharter
steveharter
commented
Aug 7, 2023

Copy link


Copy Markdown

Contributor
@stephentoub - PTAL again thanks
@steveharter
steveharter
commented
Aug 9, 2023

Copy link


Copy Markdown

Contributor
Closing this PR for v8 due to risk + schedule; ideally should be done early in the 9.0 branch. There is risk in this PR and we need more time to verify this than what we have left in v8 -- RC1 is almost done.
@Maximys thanks for this PR.
cc @JeremyKuhne - thoughts on fixing this? This issue also occurs in .NET Framework to my knowledge.
@ghostghost
locked as resolved and limited conversation to collaborators
Sep 8, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubAwaiting requested review from stephentoub
@buyaa-nbuyaa-nAwaiting requested review from buyaa-n

1 more reviewer

@steveharterstevehartersteveharter approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.ComponentModelcommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Concurrency issue in TypeDescriptor.GetConverter(type)


4 participants

@Maximys@stephentoub@steveharter@buyaa-n

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Bugfix/30024 problem with concurrency type descriptor getconverter - #85156
Closed
Maximys wants to merge 7 commits into
dotnet:mainfrom 
Maximys:bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter
Closed
Bugfix/30024 problem with concurrency type descriptor getconverter#85156
Maximys wants to merge 7 commits into
dotnet:mainfrom 
Maximys:bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter
Conversation
@Maximys
@MaximysMaximys
commented
Apr 21, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
This PR fix#30024 .

Problem was linked with race condition inside private method System.ComponentModel.TypeDescriptor.CheckDefaultProvider(Type).

I think, it'll better to change base type of WeakHashtable from Hashtable to ConcurrentDictionary<TKey,TValue> and use method GetOrAdd(TKey, Func<TKey,TValue>), but this changes can be very dangerous and I'm afraid to do them.
@ghostghost
added

area-System.ComponentModel

community-contribution
Indicates that the PR has been added by a community member
labels
Apr 21, 2023
@ghost
ghost
commented
Apr 21, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-componentmodel

See info in area-owners.md if you want to be subscribed.
Issue Details
This PR fix #30024 .

Problem was linked with race condition inside private method System.ComponentModel.TypeDescriptor.CheckDefaultProvider(Type).

I think, it'll better to change base type of WeakHashtable from Hashtable to ConcurrentDictionary<TKey,TValue>, but this changes can be very dangerous and I'm afraid to do them.
Author:Maximys
Assignees:-
Labels:
area-System.ComponentModel
Milestone:-
@Maximys
Maximysforce-pushed
the
bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter
branch
from
f3e9d88 to
4693f40CompareApril 21, 2023 11:05

This was referenced May 2, 2023

Closed


Closed

@Maximys
Maximysforce-pushed
the
bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter
branch
from
0f46857 to
e9e137eCompareMay 5, 2023 06:48
stephentoub
stephentoub
reviewed
May 6, 2023
View reviewed changes
Comment threadsrc/libraries/System.ComponentModel.TypeConverter/tests/TypeDescriptionProviderTests.cs
Outdated

stephentoub
stephentoub
requested changes
May 6, 2023
View reviewed changes
@stephentoubstephentoub
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

At quick glance, this doesn't look like a valid change.
The NodeFor method calls CheckDefaultProvider which takes the lock on s_internalSyncObject, and AddProvider calls NodeFor while holding a lock on s_providerTable. That means while holding s_providerTable the implementation can then take a lock on s_internalSyncObject. This change, though, is extending the lock in CheckDefaultProvider such that while holding the lock on s_internalSyncObject, it calls AddProvider, which will try to take a lock on s_providerTable. That means two threads could now deadlock: one holds the lock on s_providerTable and tries to acquire the lock on s_internalSyncObject, and the other holds the lock on s_internalSyncObject and tries to acquire the lock on s_providerTable.
stephentoub

This comment was marked as duplicate.

Sign in to view
@ghostghost
added
the 
needs-author-action
An issue or pull request that requires more info or actions from the author.
label
May 6, 2023
@Maximys
Maximysforce-pushed
the
bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter
branch
from
e9e137e to
92254efCompareMay 13, 2023 16:48
@ghostghost
removed
the 
needs-author-action
An issue or pull request that requires more info or actions from the author.
label
May 13, 2023
@Maximys
Maximys
commented
May 14, 2023

Copy link


Copy Markdown

ContributorAuthor
@stephentoub , thanks for your comment and attention. I had fixed this problem by ReaderWriterLockSlim and made amend of my commits.
@Maximys
Maximys
commented
May 22, 2023

Copy link


Copy Markdown

ContributorAuthor
@stephentoub ?
steveharter
steveharter
approved these changes
Jun 26, 2023
View reviewed changes
@stevehartersteveharter
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

The latest rebase\changes look like they address the feedback from @stephentoub.
@stephentoub
stephentoub
commented
Jun 26, 2023

Copy link


Copy Markdown

Member
The latest rebase\changes look like they address the feedback from @stephentoub.
I've not re-reviewed to determine whether there might still be the potential for deadlocks.
Also, what's the perf impact? It's now taking a reader lock on a path that wasn't previously locked.
@Maximys
Maximys
commented
Jun 27, 2023

Copy link


Copy Markdown

ContributorAuthor
The latest rebase\changes look like they address the feedback from @stephentoub.
I've not re-reviewed to determine whether there might still be the potential for deadlocks.
Also, what's the perf impact? It's now taking a reader lock on a path that wasn't previously locked.
@stephentoub , could you provide some info about steps for perfomance measurement for TypeDescriptor inside current repository? I usually prefer to use BenchmarkDotNet inside console application, but I don't know how to apply this way for profiling code inside .NET Runtime.
@steveharter
steveharter
commented
Jun 27, 2023

Copy link


Copy Markdown

Contributor
@stephentoub , could you provide some info about steps for perfomance measurement for TypeDescriptor inside current repository? I usually prefer to use BenchmarkDotNet inside console application, but I don't know how to apply this way for profiling code inside .NET Runtime.
We do have official benchmarks, but even those call public APIs. Would using variants of your tests with BenchmarkDotNet measure the extra read lock? If the test is very fast, you may want an inner loop within the benchmark method.
@Maximys
Maximys
commented
Jun 28, 2023

Copy link


Copy Markdown

ContributorAuthor
@stephentoub , could you provide some info about steps for perfomance measurement for TypeDescriptor inside current repository? I usually prefer to use BenchmarkDotNet inside console application, but I don't know how to apply this way for profiling code inside .NET Runtime.
We do have official benchmarks, but even those call public APIs. Would using variants of your tests with BenchmarkDotNet measure the extra read lock? If the test is very fast, you may want an inner loop within the benchmark method.
I had create special branch bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter(NEW), where I trying to add required tests. After that, I had build Runtime by
build.cmd clr+libs+libs.tests -rc release -lc release

as it's described here. And then I had trying to use
dotnet run -c Release -f net8.0 --filter "FullyQualifiedName=System.ComponentModel.Tests.PerformanceTests.GetConverter_ByMultithread_New" \ --corerun C:\runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe

command for run one of my tests, but output of this command is:
C:\Program Files\dotnet
C:\Program Files\dotnet\sdk\8.0.100-preview.5.23303.2\Sdks\Microsoft.NET.Sdk\targets\Microsoft.PackageDependencyResolution.targets(266,5): error NETSDK1005: Assets file 'C:\runtime\artifacts\obj\tasks\project.assets.json' doesn't have a target for 'net8.0'. Ensure that restore has run and that you have included 'net8.0' in the TargetFrameworks for your project.

And I can't figure out how to fix this problem. May be somebody could explain me steps for running BenchmarkDotNet inside current repository, as I asked earlier.
@Maximys
Maximys
commented
Jun 28, 2023

Copy link


Copy Markdown

ContributorAuthor
If I run already existed perfomance tests from dotnet/performance by
dotnet run -c Release -f net8.0 --filter "System.ComponentModel.Tests.Perf_TypeDescriptorTests.GetConverter" \ --corerun "C:\runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe" "C:\runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe"
command, then I receive next result:
// * Summary *
BenchmarkDotNet=v0.13.2.2052-nightly, OS=Windows 10 (10.0.19045.3086)
AMD Ryzen 5 2600, 1 CPU, 12 logical and 6 physical cores
.NET SDK=8.0.100-preview.5.23303.2
[Host] : .NET 8.0.0 (8.0.23.28008), X64 RyuJIT AVX2
Job-XYPMDP : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT AVX2
Job-FNPGHD : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT AVX2
PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:EnableUnsafeBinaryFormatterSerialization=true IterationTime=250.0000 ms
MaxIterationCount=20 MinIterationCount=15 WarmupCount=1
| Method | Job | Toolchain | typeToConvert | Mean | Error | StdDev | Median | Min | Max | Ratio | RatioSD | Allocated | Alloc Ratio |
|------------- |----------- |------------------------------------------------------------------------------------------------------------------------- |--------------------- |---------:|--------:|--------:|---------:|---------:|---------:|------:|--------:|----------:|------------:|
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassIDerived | 146.5 ns | 0.86 ns | 0.72 ns | 146.3 ns | 145.6 ns | 147.9 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassIDerived | 112.6 ns | 0.30 ns | 0.28 ns | 112.6 ns | 112.2 ns | 113.1 ns | 0.77 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassWithNoConverter | 147.3 ns | 0.45 ns | 0.43 ns | 147.3 ns | 146.5 ns | 148.1 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassWithNoConverter | 111.0 ns | 0.17 ns | 0.15 ns | 111.0 ns | 110.7 ns | 111.4 ns | 0.75 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | DerivedClass | 200.3 ns | 1.90 ns | 1.78 ns | 200.4 ns | 196.9 ns | 203.4 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | DerivedClass | 161.5 ns | 5.24 ns | 5.82 ns | 160.1 ns | 155.6 ns | 177.6 ns | 0.81 | 0.03 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | IDerived | 138.1 ns | 0.27 ns | 0.25 ns | 138.0 ns | 137.8 ns | 138.5 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | IDerived | 102.1 ns | 0.08 ns | 0.07 ns | 102.1 ns | 102.1 ns | 102.3 ns | 0.74 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeEnum | 225.1 ns | 0.80 ns | 0.74 ns | 225.3 ns | 223.8 ns | 226.1 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeEnum | 185.4 ns | 1.26 ns | 0.98 ns | 185.2 ns | 184.4 ns | 188.0 ns | 0.82 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Enum | 197.6 ns | 1.10 ns | 0.97 ns | 197.4 ns | 196.2 ns | 199.9 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Enum | 163.6 ns | 4.30 ns | 4.78 ns | 164.2 ns | 155.8 ns | 170.7 ns | 0.83 | 0.02 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Guid | 203.0 ns | 3.68 ns | 3.44 ns | 204.4 ns | 195.4 ns | 209.0 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Guid | 158.9 ns | 3.07 ns | 2.87 ns | 158.0 ns | 155.8 ns | 166.4 ns | 0.78 | 0.02 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32 | 199.0 ns | 3.99 ns | 4.60 ns | 200.4 ns | 191.5 ns | 205.8 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32 | 156.9 ns | 2.71 ns | 2.40 ns | 156.8 ns | 153.7 ns | 162.3 ns | 0.79 | 0.02 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeValueType? | 197.0 ns | 0.80 ns | 0.74 ns | 197.2 ns | 195.3 ns | 197.8 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeValueType? | 161.3 ns | 3.74 ns | 4.31 ns | 160.7 ns | 154.8 ns | 168.4 ns | 0.82 | 0.02 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32? | 207.1 ns | 5.69 ns | 6.32 ns | 206.5 ns | 196.9 ns | 220.8 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32? | 160.0 ns | 2.75 ns | 2.57 ns | 159.9 ns | 156.3 ns | 165.0 ns | 0.77 | 0.02 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-XYPMDP | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | String | 144.4 ns | 0.44 ns | 0.39 ns | 144.3 ns | 143.7 ns | 145.2 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-FNPGHD | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | String | 105.9 ns | 0.09 ns | 0.08 ns | 105.9 ns | 105.8 ns | 106.0 ns | 0.73 | 0.00 | - | NA |

that is, performance of TypeDescriptor.GetConverter has degraded by about 20%
@steveharter
steveharter
commented
Jun 28, 2023

Copy link


Copy Markdown

Contributor
that is, performance of TypeDescriptor.GetConverter has degraded by about 20%
That seems reasonable to me.
@Maximys
Maximys
commented
Jun 30, 2023

Copy link


Copy Markdown

ContributorAuthor
@stephentoub , also lets look to one more branch of my repository: 30024-problem-with-concurrency-typeDescriptor-getconverter(NEW). It contains one more little optimization which provide next result:
// * Summary *
BenchmarkDotNet=v0.13.2.2052-nightly, OS=Windows 10 (10.0.19045.3155)
AMD Ryzen 5 2600, 1 CPU, 12 logical and 6 physical cores
.NET SDK=8.0.100-preview.5.23303.2
[Host] : .NET 8.0.0 (8.0.23.28008), X64 RyuJIT AVX2
Job-ICGKTF : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT AVX2
Job-LAEOHV : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT AVX2
PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:EnableUnsafeBinaryFormatterSerialization=true IterationTime=250.0000 ms
MaxIterationCount=20 MinIterationCount=15 WarmupCount=1
| Method | Job | Toolchain | typeToConvert | Mean | Error | StdDev | Median | Min | Max | Ratio | RatioSD | Allocated | Alloc Ratio |
|------------- |----------- |------------------------------------------------------------------------------------------------------------------------- |--------------------- |---------:|--------:|--------:|---------:|---------:|---------:|------:|--------:|----------:|------------:|
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassIDerived | 123.7 ns | 0.41 ns | 0.34 ns | 123.6 ns | 123.4 ns | 124.6 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassIDerived | 109.3 ns | 0.08 ns | 0.06 ns | 109.4 ns | 109.2 ns | 109.5 ns | 0.88 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassWithNoConverter | 125.5 ns | 0.33 ns | 0.31 ns | 125.5 ns | 124.8 ns | 125.9 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | ClassWithNoConverter | 112.4 ns | 0.24 ns | 0.21 ns | 112.4 ns | 111.8 ns | 112.6 ns | 0.90 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | DerivedClass | 184.7 ns | 1.69 ns | 1.50 ns | 184.2 ns | 182.6 ns | 187.5 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | DerivedClass | 161.5 ns | 1.68 ns | 1.57 ns | 160.9 ns | 159.4 ns | 164.5 ns | 0.88 | 0.01 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | IDerived | 119.7 ns | 0.51 ns | 0.47 ns | 119.7 ns | 118.8 ns | 120.4 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | IDerived | 102.3 ns | 0.07 ns | 0.06 ns | 102.3 ns | 102.1 ns | 102.4 ns | 0.85 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeEnum | 204.4 ns | 0.52 ns | 0.48 ns | 204.4 ns | 203.3 ns | 205.0 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeEnum | 183.8 ns | 0.20 ns | 0.17 ns | 183.8 ns | 183.3 ns | 184.0 ns | 0.90 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Enum | 183.8 ns | 1.75 ns | 1.64 ns | 184.4 ns | 180.3 ns | 186.0 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Enum | 162.6 ns | 2.01 ns | 1.88 ns | 162.3 ns | 159.9 ns | 166.3 ns | 0.88 | 0.01 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Guid | 185.3 ns | 0.85 ns | 0.79 ns | 185.5 ns | 183.2 ns | 186.1 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Guid | 162.5 ns | 0.90 ns | 0.85 ns | 162.7 ns | 159.9 ns | 163.4 ns | 0.88 | 0.00 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32 | 181.4 ns | 1.72 ns | 1.61 ns | 181.8 ns | 178.5 ns | 183.6 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32 | 161.1 ns | 1.30 ns | 1.22 ns | 160.9 ns | 158.7 ns | 162.7 ns | 0.89 | 0.01 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeValueType? | 179.8 ns | 2.63 ns | 2.46 ns | 180.2 ns | 175.7 ns | 183.4 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | SomeValueType? | 160.5 ns | 2.12 ns | 1.98 ns | 160.3 ns | 157.5 ns | 163.9 ns | 0.89 | 0.02 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32? | 179.4 ns | 2.73 ns | 2.55 ns | 179.5 ns | 175.8 ns | 184.2 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | Int32? | 157.7 ns | 1.77 ns | 1.65 ns | 157.1 ns | 155.4 ns | 160.5 ns | 0.88 | 0.01 | - | NA |
| | | | | | | | | | | | | | |
| GetConverter | Job-ICGKTF | \runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | String | 124.0 ns | 0.49 ns | 0.46 ns | 124.2 ns | 123.5 ns | 124.6 ns | 1.00 | 0.00 | - | NA |
| GetConverter | Job-LAEOHV | \runtime_without_my_fix\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\CoreRun.exe | String | 109.4 ns | 0.06 ns | 0.05 ns | 109.4 ns | 109.3 ns | 109.4 ns | 0.88 | 0.00 | - | NA |

As you can see, perfomance degradation has decreased to 10% after commit 92a2d245958ce41e05398580c0ea45419e05effe. May be it's ok? What do you think?
@steveharter
steveharter
requested a review
from buyaa-nJuly 5, 2023 17:19
@Maximys
Maximysforce-pushed
the
bugfix/30024-problem-with-concurrency-typeDescriptor-getconverter
branch
from
92254ef to
1f20655CompareAugust 4, 2023 04:25
steveharter
steveharter
reviewed
Aug 4, 2023
View reviewed changes
Comment threadsrc/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs
Outdated

Comment threadsrc/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs
Outdated

Comment threadsrc/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs
Comment threadsrc/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs
@steveharter
steveharter
self-requested a review
August 7, 2023 15:11
@steveharter
steveharter
commented
Aug 7, 2023

Copy link


Copy Markdown

Contributor
@stephentoub - PTAL again thanks
@steveharter
steveharter
commented
Aug 9, 2023

Copy link


Copy Markdown

Contributor
Closing this PR for v8 due to risk + schedule; ideally should be done early in the 9.0 branch. There is risk in this PR and we need more time to verify this than what we have left in v8 -- RC1 is almost done.
@Maximys thanks for this PR.
cc @JeremyKuhne - thoughts on fixing this? This issue also occurs in .NET Framework to my knowledge.
@ghostghost
locked as resolved and limited conversation to collaborators
Sep 8, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubAwaiting requested review from stephentoub
@buyaa-nbuyaa-nAwaiting requested review from buyaa-n

1 more reviewer

@steveharterstevehartersteveharter approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.ComponentModelcommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Concurrency issue in TypeDescriptor.GetConverter(type)


4 participants

@Maximys@stephentoub@steveharter@buyaa-n