Skip to content
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>: Skip reset in normal_distribution::operator() with custom params #4618

Merged
merged 7 commits into from
Apr 26, 2024
Merged

<random>: Skip reset in normal_distribution::operator() with custom params #4618

merged 7 commits into from
Apr 26, 2024

Conversation

horenmar
Copy link
Contributor

normal_distribution works by computing a pair of coefficients that are multiplied with the stddev to compute how far (and in which direction) the result is from the mean. These coefficients are not reliant on the actual stddev (or mean), so there is no reason to avoid reusing one computed during last invocation, even if it was with different params.

AFAIK usage of this overload is rare (personally I think this overload should've never been standardized), the old code still needlessly penalized it, in the worst case halving the performance.

…arams

`normal_distribution` works by computing a pair of coefficients that
are multiplied with the stddev to compute how far (and in which direction)
the result is from the mean. These coefficients are not reliant on the
actual stddev (or mean), so there is no reason to avoid reusing one
computed during last invocation, even if it was with different params.

AFAIK usage of this overload is rare (personally I think this overload
should've never been standardized), the old code still needlessly penalized
it, in the worst case halving the performance.
@horenmar horenmar requested a review from a team as a code owner April 22, 2024 12:25
@horenmar
Copy link
Contributor Author

I am not sure whether this needs a test, and where to add it. The organization of tests seems to be oriented around papers/github issues, but this has none... I guess I could use the github PR number instead, now that it is open.

@AlexGuteniev
Copy link
Contributor

I guess I could use the github PR number instead, now that it is open

Sounds good to me.

An alternative could be squeezing into an existing test, but I don't see a suitable tests/std/tests/* one, and tests/tr1/tests/* aren't honored nowadays.

@StephanTLavavej StephanTLavavej changed the title <random>: Skip reset in normal_distribution::operator() with custom params <random>: Skip reset in normal_distribution::operator() with custom params Apr 22, 2024
@StephanTLavavej StephanTLavavej added the performance Must go faster label Apr 22, 2024
@StephanTLavavej StephanTLavavej self-assigned this Apr 22, 2024
@StephanTLavavej
Copy link
Member

This is awesome, thank you! 😻 I pushed a conflict-free merge with main followed by tiny code review nitpicks, nothing that affects behavior.

@StephanTLavavej
Copy link
Member

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

@StephanTLavavej StephanTLavavej merged commit 6b54464 into microsoft:main Apr 26, 2024
39 checks passed
@StephanTLavavej
Copy link
Member

Thanks again for improving performance and simplifying code! 🚀 ✨ 😻

@horenmar horenmar deleted the improve-normal-distribution-performance branch April 27, 2024 06:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Must go faster
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants