Remove empty Threadpool .cctors

[benaadams]

Do ThreadPoolGlobals, QueueUserWorkItemCallback and _ThreadPoolWaitOrTimerCallback need empty static constructors?

[stephentoub]

Do ThreadPoolGlobals, QueueUserWorkItemCallback and _ThreadPoolWaitOrTimerCallback need empty static constructors?

I expect these are there because in a world of Security{Safe}Critical and SecurityTransparent, initializing a static SecurityCritical field can only be done from a Security{Safe}Critical static cctor, and to attribute it appropriately you need the explicit cctor. For mscorlib in CoreCLR, I'm not sure if that matters. @AlexGhiondea or @jkotas can probably comment more authoritatively.

[benaadams]

Ah, that would make sense.

[benaadams]

@stephentoub would something like this work? Have put the initializers in a SecuritySafeCritical path for user code (QueueUserWorkItem) which go via QueueUserWorkItemHelper to EnsureVMInitialized

[stephentoub]

@ericeil might be the best to comment.

[ericeil]

This seems reasonable to me.

[benaadams]

Rebased and squished

[AlexGhiondea]

I am not sure I understand the advantage of this change.

Sure, we don't have a static constructor that does seems to do nothing, but this will make it way harder to understand the way transparency works in those cases.
And I would argue that the static constructor does something: sets up he security model.

While for now we found the code that called it and marked it appropriately, i think that has a couple of problems:

• now the caller's transparency has changed so we might have to potentially update its callers
• in the future, we are going to have to ensure that each caller has the right attribute applied which is very easy to forget to do
• the compiler already generates the static constructor and combine the empty one with the one that does the field initializer.

Can you please explain what advantage this change has over leaving it as-is?

[stephentoub]

Can you please explain what advantage this change has over leaving it as-is?

There is cost to having an explicit static cctor, in that the C# compiler will then not mark the type as beforefieldinit, and the backend compiler will be forced to add initialization checks to static methods on the type. It's something static analysis rules have checked for: https://msdn.microsoft.com/en-us/library/ms182275.aspx

I don't know whether it makes a measurable difference in this particular case.

[benaadams]

There is a gotcha that the first function that is called and does field access has the initialization checks compiled into it; while any later function does not. Seeing if I can rearrange.

[jkotas]

This does not mattet. mscorlib is always expected to be precompiled using crossgen in production - all static access has this check in precompiled code.

[benaadams]

😭 might as well close then

[jkotas]

I just meant that reordering does not matter. There may be still benefit on your change by avoiding the cctor check in static methods that do not access any fields - have not looked whether there are any.

[benaadams]

@jkotas can initialize the four delegates to non-capturing lambdas, drop some of the static functions and a lot of the casts?

That work in the native realm?

[benaadams]

Was the behaviour correct to not set ThreadPoolGlobals.vmTpInitialized = true; at this point previously?

[benaadams]

Last change drops the auto.cctor entirely (which is different from the explicit non-beforefieldinit .cctor) and may mean no runtime guards are needed on the static fields?

[benaadams]

From

To

[AlexGhiondea]

@benaadams do you have some performance numbers to show what this change will do?

[benaadams]

@AlexGhiondea these functions are called several million times a second so it is a hot path for performance.

However, I haven't managed to validate it yet. I disassembled and stepped through both versions (non-native image) and this version has slightly fewer instructions but there isn't that much change due to the magic of the jitter.

I'm going to have to compare and benchmark the native images which will take a bit longer.

[benaadams]

Closing, measured and integrated in #5943