Switch CoreCLR finalizer loop to C# #103501
Conversation
|
Tagging subscribers to this area: @mangod9 |
jkotas
force-pushed
the
managed-run-finalizers
branch
2 times, most recently
from
3d5877f
to
aa17778
Compare
|
@janvorli Could you please run your ad-hoc micro-benchmark (#103425 (comment)) on this change? |
jkotas
force-pushed
the
managed-run-finalizers
branch
2 times, most recently
from
5997715
to
43daf2d
Compare
jkotas
force-pushed
the
managed-run-finalizers
branch
from
43daf2d
to
0ca1506
Compare
|
@jkotas the performance is about the same as before this change (very slightly worse, 2-5%). |
|
|
||
| _mayNeedResetForThreadPool = false; | ||
|
|
||
| const string FinalizerThreadName = ".NET Finalizer"; |
There was a problem hiding this comment.
I am not sure I understand why we are doing this rename. Aren't we always calling the RunFinalizers on the finalizer thread that is already named as such?
There was a problem hiding this comment.
User code can change the thread name. This is going to set it back if that happens.
Historically, threadpool threads and finalizer thread had code that tried to reset a few properties like name if they got modified by the user code. I never liked that behavior, but I am keeping for compat here. There are many other ways that the user code can damage thread and finalizers threads that we are not accounting for.
The unmanaged version that I am deleting in this PR had a bug: https://github.com/dotnet/runtime/pull/103501/files#diff-f5835c4b5fd134e52b4127bb4ffb7e5ad439673a429dc7ea46d53e7a5bca0529L7735 . It reset the finalizer thread name to NULL, not the name that we use for finalizer threads.
|
For my edification, what does this buy us? just the general "more C# rather than C is better" charge? |
Before this change, Windows x86/x64 had hand-written assembly code to minimize finalizer loop overhead. The exception handling bug was related to this hand-written assembly code. Switching to managed implementation allowed us to fix the exception handling bug, get rid of the x86/x64 hand-written assembly code, and improve perf outside Windows x86/x64. An alternative fix would be to keep writing more platform-specific code to fix the exception handling bug, and potentially keep writing even more platform-specific code to get other platforms to perf parity with Windows x86/x64. |
|
#103385 is the EH issue that this contributes to. |
|
Excellent, thanks |
Context: #103425 (comment)