Use Random.Shared

[martincostello]

Use the new Random.Shared property to remove the need to allocate a new instance when targeting net6.0.

[roji]

Thanks for submitting this.

For the change in ExecutionStrategy, I'm not sure this is appropriate. Random.Shared was explicitly meant for one-off random generation, to allow people to avoid managing an instance of Random themselves (see https://github.com/dotnet/runtime/pull/50297/files). But ExecutionStrategy already has this field (and uses it repeatedly, not one-off), so none of the advantages apply here (this doesn't simplify any code).

In addition, the point of Random.Shared is also to ensure thread-safety when accessed concurrently. This again isn't the case for ExecutionStrategy, which itself isn't supposed to be thread-safe. The thread-safe implementation is very likely to (slightly) degrade perf.

I do get that this reduces an allocation. However, it would be better to simply recycle ExecutionStrategy itself - along with its dependent Random instance. This is something I looked into as part of #23611, but didn't get around to doing.

However, the changes in the tests definitely do make sense to me. If you agree with the above, could you exclude the ExecutionStrategy change?

[martincostello]

For the change in ExecutionStrategy, I'm not sure this is appropriate. Random.Shared was explicitly meant for one-off random generation, to allow people to avoid managing an instance of Random themselves (see https://github.com/dotnet/runtime/pull/50297/files). But ExecutionStrategy already has this field (and uses it repeatedly, not one-off), so none of the advantages apply here (this doesn't simplify any code).

I was working on the basis it would remove the need for the allocating of a new Random for each instance of an execution strategy and just use the shared one (albeit it being a minor saving), but I'm happy to revert that part if required.

[roji]

@martincostello yeah, reducing the allocation does make sense. But as I added above:

It would be better to simply recycle ExecutionStrategy itself - along with its dependent Random instance. This is something I looked into as part of #23611, but didn't get around to doing.

So I think it's better to revert the ExecutionStrategy change, and recycle the entire object in a separate, later effort.

[martincostello]

Ah sorry, I must have skipped over that sentence. Sure, I'll take that part out of the PR then.

[roji]

Thanks @martincostello (FWIW you didn't skip over the sentence - I edited and added it right after posting, but you were faster reading :)).

[ghost]

Hello @roji!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.