Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improved RandomHelper #20531

Open
wants to merge 6 commits into
base: dev
Choose a base branch
from

Conversation

MarkCiliaVincenti
Copy link
Contributor

Internally uses Random.Shared when available (.NET 6.0), otherwise uses a thread-safe random.

Furthermore, list shuffling uses the Fisher-Yates algorithm for better performance.

@maliming maliming self-requested a review August 16, 2024 01:01
@maliming maliming added this to the 9.0-preview milestone Aug 16, 2024
@MarkCiliaVincenti
Copy link
Contributor Author

@maliming I'm OK with rewriting this in such a way to avoid the dependency, basically copying and pasting the code in and adding a comment at the top for credit, and we can leave them public as well. Would that work for you?

@maliming
Copy link
Member

@maliming I'm OK with rewriting this in such a way to avoid the dependency, basically copying and pasting the code in and adding a comment at the top for credit, and we can leave them public as well. Would that work for you?

That would be great. 👍

@MarkCiliaVincenti
Copy link
Contributor Author

@maliming I'm OK with rewriting this in such a way to avoid the dependency, basically copying and pasting the code in and adding a comment at the top for credit, and we can leave them public as well. Would that work for you?

That would be great. 👍

I tried to do this but unfortunately this should be multi-targeting a framework version that is not being targeted in Volo.Abp.Core, namely .NET 6.0 where Random.Shared was introduced. It will also use the internal Shuffle of .NET 8.0 but that's not an issue as .NET 8.0 is currently being targeted, but will it remain being targeted?

It would be best to have these micro optimizations handled externally in a dedicated and controlled assembly.

Therefore my recommendation is to optimally take the micro-dependencies.

@maliming
Copy link
Member

maliming commented Oct 15, 2024

@MarkCiliaVincenti
Copy link
Contributor Author

hi @MarkCiliaVincenti

These changes will be easier to maintain and understand. What do you think?

https://github.com/abpframework/abp/pull/21089/files

https://stackoverflow.com/questions/1287567/is-using-random-and-orderby-a-good-shuffle-algorithm/1287572#1287572

Also applies to #20530

There are some changes that should be done here.

  1. I don't quite like the yield return and returning an IEnumerable because there's a risk that someone keeps it as an ienumerable and it's reprocessed every time it's accessed... and in such case the shuffled version would be different on top. Very dangerous.

  2. Instead of using a static Random instance and locking on it, you can optimize it with a ThreadLocal. Take a look at https://github.com/MarkCiliaVincenti/ThreadSafeRandomizer/blob/master/ThreadSafeRandomizer/ThreadSafeRandom.cs

  3. GenerateRandomizedList now has a breaking change. With the current code, the input (items) is not shuffled, but with the version in Use Random.Shared if available. #21089 items will get shuffled.

  4. NET 8.0 already includes a shuffle and we should use that.

If you want I can modify this PR and import the two libraries here in separate files. They won't be fully optimized due to the lack of extra targeting, but it will work.

As for #20530, please discuss there. The changes absolutely need to be in a different assembly there.

@maliming
Copy link
Member

Thanks. I will check it later.

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.

2 participants