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

Improve shuffling algorithm of connection list #634

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Hurukawa2121
Copy link

@Hurukawa2121 Hurukawa2121 commented Oct 26, 2024

I propose implementing the Fisher-Yates shuffle algorithm to improve the shuffling of the connection target list.

You can run this simple simulation to intuitively understand how much bias is produced by the current method and how much the Fisher-Yates shuffle algorithm improves it.

Issue:

In the current shuffle algorithm, each element l->array[i] is swapped with a random element at index j in the entire array.
However, this method does not produce all permutations uniformly, resulting in a bias where some permutations occur more frequently than others.

Solution:

To ensure all permutations are generated with equal probability, I implemented the Fisher-Yates shuffle algorithm.

Intuitive Explanation of Uniformity:

In this algorithm, we start from the end of the array and move towards the beginning.
At each step i, we randomly select an index j between 0 and i (inclusive) and swap l->array[i] with l->array[j]. This means we're randomly choosing an element from the unshuffled portion and placing it at position i.
Since there's only one way to obtain each permutation through these swaps, all permutations occur with equal probability in theory.

Proof of Uniformity:

At each iteration i, there are exactly i + 1 choices for j, so the probability of selecting any particular j is $\frac{1}{i + 1}$.
Since the choices at each iteration are independent, the total probability of any specific permutation is:

$P = \prod_{k=1}^n \frac{1}{k} = \frac{1}{n!}$

Therefore, it is concluded that each permutation occurs with equal probability.

Thank you for your contribution

You are welcome to open PR, but they are used for discussion only. All
patches must eventually go to the openvpn-devel mailing list for review:

Please send your patch using git-send-email. For example to send your latest commit to the list:

$ git send-email [email protected] HEAD~1

For details, see these Wiki articles:

@cron2
Copy link
Contributor

cron2 commented Nov 6, 2024

This looks interesting.

To progress, please add some comments to the new code explaining what this does ("Shuffle with Fisher-Yates algorithm which has less bias to ...."). Otherwise this creates just code that is not meaningful to the next person looking at it.

Then, send the patch to the openvpn-devel list, or to our gerrit. We do not merge patches from GH.

thanks ;-)

@Hurukawa2121
Copy link
Author

Hurukawa2121 commented Nov 15, 2024

Thanks for your feedback :)

I added comments to the new code explaining why the shuffling algorithm reduces a bias.

Also, I've just sent the patch to the openvpn-devel list as you suggested.
I apologize for mistakenly sending some emails.

@schwabe
Copy link
Contributor

schwabe commented Nov 15, 2024

Your comments don't seem to be in the files changed tab, did you forget to push your changes?

@Hurukawa2121
Copy link
Author

Hurukawa2121 commented Nov 16, 2024

I misunderstood his advice and added the comments to the PR description.

I have now updated the code with the necessary comments.
I have also resent the email.

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants