<random>: Improve performance of poisson_distribution constructor

[gcerretani]

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.

[StephanTLavavej]

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 12.0 is fairly obvious so I don't think we need to extract it into a named constant. (I personally would have introduced a static constexpr double _Small_threshold but it isn't worth resetting testing.)

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!

[AlexGuteniev]

This leaves the unused fields uninitialized, though it is unclear to me if this is acceptable or not.

I'm actually not sure. My concern is that param can be copied,

Can copying of uninitialized double signal?

See also: https://devblogs.microsoft.com/oldnewthing/20080702-00/?p=21773

[gcerretani]

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 _Small_poisson_distribution::_Init, as it also contains a double. Alternatively, we could consider overloading _Init with zero arguments to avoid calling exp in such cases.

Would you prefer that I add another commit with these suggestions, or will you handle this on your end?

[AlexGuteniev]

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.

[gcerretani]

@gcerretani I cannot push to the https://github.com/caenspa/STL repo. Is it yours under a different GitHub account?

Oh, it's an organization account of my company. I've assigned you write permission, let me know if it works!

[StephanTLavavej]

Thanks for granting me access; I pushed commits to extract the threshold constant, and add 0.0 data member initializers for peace of mind. This managed to push your PR slightly over the "de minimis" threshold, so the CLA Bot has asked you to agree to our Contributor License Agreement.

[gcerretani]

@microsoft-github-policy-service agree company="CAEN SpA"

[frederick-vs-ja]

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.

[gcerretani]

I add, as a reminder, that probably such an optimization could be done also for other distrubutions. In std::binomial_distribution, for example, the fields _Sqrt, _Logp and _Gx1 are only used if _Tx >= 25 && _Mean >= 1.0.

[StephanTLavavej]

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

[StephanTLavavej]

Thanks for noticing and improving the performance here, and congratulations on your first microsoft/STL commit! 😻 🐈 🐈‍⬛

[AlexGuteniev]

After looking into #5442 , I'd like to raise ABI question here.

Did the threshold never change since the beginning of the current ABI?

[StephanTLavavej]

The threshold has never changed since Dinkumware delivered this code to us.