Repository navigation
fix(peer): stop sleeping while generating a GUID on Windows - #81
Conversation
Get64BitUniqueRandomNumber harvested timer jitter on Windows by calling RakSleep(1) eight times. A Windows Sleep(1) lasts a whole timer tick (~15.6 ms by default), so every RakPeer took ~125 ms to construct. That was most of the unit suite's run time on Windows, where tests on the simulated network should take no time at all. The jitter added nothing: the value is already made unique by the OS entropy source and the per-process counter mixed in afterwards, which is all every other platform uses. Windows Debug unit suite: 32.7 s -> 3.0 s in one process, and 5.0 s -> 1.8 s under ctest -j 8.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe Windows GUID-generation path no longer uses sleep cycles to mix timer jitter. New tests check generation time and verify that repeated values are nonzero, differ from the unassigned GUID value, and are unique within a test run. ChangesGUID generation
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 3 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: removing sleeps from Windows GUID generation. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
- Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
A rabbit counts each GUID with care,
No sleepy cycles slow the hare.
The clock and entropy join the run,
Each value checks against the one.
Quick paws test the sequence through,
And find each number fresh and new.
Comment @coderabbitai help to get the list of available commands.
1 participant
Why
Every
RakPeergenerates its GUID in its constructor viaGet64BitUniqueRandomNumber. On Windows that function harvested timer jitter by callingRakSleep(1)eight times. A WindowsSleep(1)lasts a whole timer tick (about 15.6 ms by default), so constructing one peer cost about 125 ms. Unit tests on the simulated network create two or more peers each, so they took about 250 ms each on Windows and 0 ms on Linux. That was most of the Windows unit suite's run time.What
The jitter loop is removed. The value was already made unique by the OS entropy source (
std::random_device) and the per-process counter mixed in afterwards, which is exactly what every other platform uses. Non-Windows code is unchanged.Results (Windows Debug)
UnitTests.exe, one processctest -L unit -j 8Tests
PeerGuid.UniqueRandomNumbersAreDistinctBackToBack(unit): 10,000 back-to-back values are all distinct, and none is 0 or the unassigned sentinel. Before this change it took 20 minutes on Windows; now it takes 7 ms.PeerGuidTiming.GeneratingGuidsDoesNotSleep(integration, since it reads the wall clock): 256 GUIDs in under 100 ms. On the old code, 64 GUIDs took 7.9 s. Putting a singleRakSleep(1)back makes it fail: 256 took about 3.97 s. The bound still catches a sleep at a 1 ms timer resolution, because 256 sleeps would take at least 256 ms.Summary by CodeRabbit