random: Embed the Mt19937 and CombinedLCG state within the module globals #13579
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…bals
These are always dynamically allocated in GINIT, thus always take up memory. By
embedding them here we can avoid the dynamic allocation and additional pointer
indirection accessing them.
The test script:
<?php
for ($i = 0; $i < 9999999; $i++) mt_rand(1, 100);
Appears to run slightly faster with this change applied: Before this change it
always ran in just over 3 seconds, after this change I was also seeing times
below 3 seconds. Howver results are too close and too jittery to state this
performance improvement as a fact.
zeriyoshi
approved these changes
Mar 4, 2024
|
For reference: This fails with MSAN: https://github.com/php/php-src/actions/runs/8165488968/job/22322762350. I'll fix this tonight. |
TimWolla
added a commit
to TimWolla/php-src
that referenced
this pull request
Mar 6, 2024
…lt_status()` This is a follow-up fix for phpGH-13579. The issue was detected in the nightly MSAN build.
TimWolla
added a commit
to TimWolla/php-src
that referenced
this pull request
Mar 12, 2024
…lt_status()` (php#13608) This is not just an issue due to missing initialization since moving the state struct directly into the module globals. In earlier versions changing the mode to `MT_RAND_PHP` within a single request would also affect the mode for subsequent requests. Original commit message follows: This is a follow-up fix for phpGH-13579. The issue was detected in the nightly MSAN build. (cherry picked from commit bf0abd1)
TimWolla
added a commit
that referenced
this pull request
Mar 13, 2024
…lt_status()` (#13690) This is not just an issue due to missing initialization since moving the state struct directly into the module globals. In earlier versions changing the mode to `MT_RAND_PHP` within a single request would also affect the mode for subsequent requests. Original commit message follows: This is a follow-up fix for GH-13579. The issue was detected in the nightly MSAN build. (cherry picked from commit bf0abd1)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Somewhat related to #13577
These are always dynamically allocated in GINIT, thus always take up memory. By embedding them here we can avoid the dynamic allocation and additional pointer indirection accessing them.
The test script:
Appears to run slightly faster with this change applied: Before this change it always ran in just over 3 seconds, after this change I was also seeing times below 3 seconds. Howver results are too close and too jittery to state this performance improvement as a fact.