Skip to content

Conversation

@erozenfeld
Copy link
Member

  1. The timer workaround is no longer needed since EventPipe file polling
    was removed in Removing EventPipe file polling (EventPipeController+Timer) coreclr#24225

  2. Adjusting GetCurrentProcessorId caching to different environments. runtime#467 introduced a change that
    causes pmi non-determinism.
    System.Threading.Thread.s_isProcessorNumberReallyFast can have different
    values on two invocations of the process on the same machine.
    (https://github.com/dotnet/runtime/blob/aedf8f52006619ef5d4eca65d79f42cc4b7bc402/src/coreclr/src/System.Private.CoreLib/src/System/Threading/Thread.CoreCLR.cs#L502)
    That causes non-determinism in generated code in methods
    inlining System.Threading.Thread.GetCurrentProcessorId()
    (https://github.com/dotnet/runtime/blob/aedf8f52006619ef5d4eca65d79f42cc4b7bc402/src/coreclr/src/System.Private.CoreLib/src/System/Threading/Thread.CoreCLR.cs#L492-L498)
    The workaround is to set the value of
    System.Threading.Thread.s_isProcessorNumberReallyFast to true via
    reflection.

1. The timer workaround is no longer needed since EventPipe file polling
was removed in dotnet/coreclr#24225

2. dotnet/runtime#467 introduced a change that
causes pmi non determinism.
System.Threading.Thread.s_isProcessorNumberReallyFast can have different
values on two invocations of the process on the same machine.
(https://github.com/dotnet/runtime/blob/aedf8f52006619ef5d4eca65d79f42cc4b7bc402/src/coreclr/src/System.Private.CoreLib/src/System/Threading/Thread.CoreCLR.cs#L502)
That causes non-determinism in generated code in methods'
inlining System.Threading.Thread.GetCurrentProcessorId()
(https://github.com/dotnet/runtime/blob/aedf8f52006619ef5d4eca65d79f42cc4b7bc402/src/coreclr/src/System.Private.CoreLib/src/System/Threading/Thread.CoreCLR.cs#L492-L498)
The workaround is to set the value of
System.Threading.Thread.s_isProcessorNumberReallyFast to true via
reflection.
@erozenfeld
Copy link
Member Author

@AndyAyersMS @sandreenko @dotnet/jit-contrib PTAL

Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

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

Thanks!

@erozenfeld erozenfeld merged commit a4621f1 into dotnet:master Mar 20, 2020
@erozenfeld erozenfeld deleted the GetCurrentProcessorIdDeterminism branch March 20, 2020 23:56
erozenfeld added a commit to erozenfeld/jitutils that referenced this pull request Jun 19, 2020
I added a workaround for disassembly non-determinism in dotnet#255.
See dotnet#255 for details of what causes the non-determinism.
It turns it relied on a reflection hole that allowed
init-only static fields to be modified after static constructor has
been called. That hole was fixed in dotnet/runtime#37849
so the workaround is no longer valid and causes an exception from pmi.

I don't see a way to work around the non-determinism without changing
framework code so for now I'm just reverting the workaround. Unfortunately, that
means that we can get non-deterministic disassembly for any method that inlines
System.Threading.Thread.GetCurrentProcessorId.

Fixes dotnet#271.
erozenfeld added a commit that referenced this pull request Jun 19, 2020
I added a workaround for disassembly non-determinism in #255.
See #255 for details of what causes the non-determinism.
It turns it relied on a reflection hole that allowed
init-only static fields to be modified after static constructor has
been called. That hole was fixed in dotnet/runtime#37849
so the workaround is no longer valid and causes an exception from pmi.

I don't see a way to work around the non-determinism without changing
framework code so for now I'm just reverting the workaround. Unfortunately, that
means that we can get non-deterministic disassembly for any method that inlines
System.Threading.Thread.GetCurrentProcessorId.

Fixes #271.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants