Skip to content

Conversation

mathieucarbou
Copy link
Member

@mathieucarbou mathieucarbou commented Sep 17, 2025

rand() is called at 2 places, and we do not care about having a good distribution of random numbers.
So I think we can easily use a more appropriate implementation (xor shift) which can be enough for our need and definitely way faster than calling rand()

Copy link

@willmmiles willmmiles left a comment

Choose a reason for hiding this comment

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

Generally looks OK to me. It's too bad there isn't a fast PRNG reliably available via std/the system libraries; it's a bit of a shame to have to keep our own state for it.

That said, my preference would be to remove the stochastic behaviour entirely. I still have an unfinished patch that limits the number of outstanding poll events to 1 or 2 per client. The initial implementation added a queued polls counter to each AsyncClient, which is O(1) but must be managed under the mutex. An alternative would be to scan the queue (O(N)) and do a quick count of matching events before adding a new poll event, which trades CPU to save a little memory and complexity. In both cases it ensures that no unlucky client gets completely starved of poll events.

@mathieucarbou
Copy link
Member Author

That said, my preference would be to remove the stochastic behaviour entirely.

I agree ;-)

@mathieucarbou mathieucarbou merged commit 4c0357e into main Sep 17, 2025
25 checks passed
@me-no-dev
Copy link
Member

ESP32 chips have actual hardware for random. That would be the proper fix here. Why was this merged so fast?

@me-no-dev
Copy link
Member

@willmmiles
Copy link

I considered suggesting it, but unfortunately there doesn't seem to be a nonblocking call to access the random hardware? We do not need cryptographic quality randomness, and we definitely do not want to block waiting for more entropy.

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