CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
<random>
: Improve performance of poisson_distribution
constructor
#5411
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
<random>
: Improve performance of poisson_distribution
constructor
#5411
Conversation
β¦in case of small mean, since those values are never used.
<random>
: Improve performance of poisson_distrubution
constructor
Looks good to me, thanks. π» The uninitialized fields are slightly concerning, but they aren't printed by the streaming operator or make any observable difference. The magicalness of the We merge PRs simultaneously to our GitHub and MSVC-internal repos in a semi-manual process, batched up to save time. Your PR will be part of the next batch, possibly this week or more likely next week. I'll post comments here as I prepare your PR for merging! |
I'm actually not sure. My concern is that Can copying of uninitialized See also: https://devblogs.microsoft.com/oldnewthing/20080702-00/?p=21773 |
Thank you for your feedback. I have implemented a minimal change, as I am not entirely familiar with your code policy. In my opinion, we could initialize all unused parameters to zero if they are not used and always invoke Would you prefer that I add another commit with these suggestions, or will you handle this on your end? |
To be clear, I'm not a maintainer, @StephanTLavavej is the one. I'm just stating potential concern. I'm not sure if there's a need to initialize variables. But if there is, I yield the decision how to do this to Stephan. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Oh, it's an organization account of my company. I've assigned you write permission, let me know if it works! |
Thanks for granting me access; I pushed commits to extract the threshold constant, and add |
@microsoft-github-policy-service agree company="CAEN SpA" |
_Ty1 _Sqrt = 0.0; | ||
_Ty1 _Logm = 0.0; | ||
_Ty1 _Gx1 = 0.0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No change requested. Now it seems clear that the member group {_Sqrt
, _Logm
, _Gx1
} and the _Small
member never logically coexist. So perhaps we can store them into a union with _Mean
being the tag, which makes param_type
smaller. Although such change is ABI breaking and can only be done since vNext.
I add, as a reminder, that probably such an optimization could be done also for other distrubutions. In |
I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed. |
<random>
: Improve performance of poisson_distrubution
constructor<random>
: Improve performance of poisson_distribution
constructor
Thanks for noticing and improving the performance here, and congratulations on your first microsoft/STL commit! π» π πββ¬ |
After looking into #5442 , I'd like to raise ABI question here. Did the threshold never change since the beginning of the current ABI? |
The threshold has never changed since Dinkumware delivered this code to us. |
This patch simplifies the construction of
std::poisson_distribution
by avoiding the computation of members that are not used when the mean is small, and vice versa.This leaves the unused fields uninitialized, though it is unclear to me if this is acceptable or not.