Skip to content

runtime: added standard library compatibility to RandomGenerator#9053

Merged
lizan merged 2 commits intoenvoyproxy:masterfrom
SaveTheRbtz:random_stdlib
Dec 2, 2019
Merged

runtime: added standard library compatibility to RandomGenerator#9053
lizan merged 2 commits intoenvoyproxy:masterfrom
SaveTheRbtz:random_stdlib

Conversation

@SaveTheRbtz
Copy link
Contributor

Description:
This adds stdlib's glue code to the Runtime::RandomGenerator implementing UniformRandomBitGenerator requirements. This allows to use RandomGenerator with functions like std::sample, std::shuffle, etc.
It's up to the maintainers' taste whether this code belongs here or in a separate "Adapter" class.

Risk Level: Low
Testing: Added unit test using Runtime::RandomGeneratorImpl with std::shuffle.

@junr03
Copy link
Member

junr03 commented Nov 18, 2019

@dnoe do you mind taking a look at this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we ever foresee this changing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, but it is a part of UniformRandomBitGenerator[1] interface. If it looks ugly/weird inside this class, I can move it to a separate "adaptor" class.

[1] https://en.cppreference.com/w/cpp/named_req/UniformRandomBitGenerator

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm clang-tidy borked but "passed" the PR cc @lizan

...
Running clang-tidy-diff against master branch...
From https://github.com/envoyproxy/envoy
 * branch            master     -> FETCH_HEAD
fatal: ambiguous argument 'master..HEAD': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
No relevant changes found.

(it should be flagging this line because codebase convention is for type aliases to be CamelCase, but can be overridden with // NOLINT(readability-naming-identifier) for cases like this when conforming to a library such as here)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

opened #9140 to fix clang-tidy

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added lint comment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this also implicitly exercise min() and max()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, std::uniform_int_distribution[1] used underneath std::shuffle uses min() and max()

[1] https://en.cppreference.com/w/cpp/numeric/random/uniform_int_distribution.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has obvious flakey test appearance, although the probability of a flake would presumably be very low (https://en.wikipedia.org/wiki/Bogosort comes to mind 😃 ). It's of course also possible that a perfectly correct random number generator results in the same shuffle occurring two times in a row, but this does seem extremely unlikely.

In practice, if you can execute this test a few times with some very high ``--runs_per_test` value like 10000+ and no problems maybe it's OK. Have you tried that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, theoretically there is ~10000/100! chance of that test failing which is practically impossible. Nevertheless I did run it overnight to confirm that this won't happen:

  Stats over 10000 runs: max = 20.2s, min = 5.7s, avg = 8.7s, dev = 2.8s

Signed-off-by: Alexey Ivanov <rbtz@dropbox.com>
dnoe
dnoe previously approved these changes Nov 25, 2019
Copy link
Contributor

@dnoe dnoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. FYI, next time, don't force push. It messes things up in the GH review UI.

@lizan can you take a senior maintainer pass and merge if it looks good to you?

Signed-off-by: Alexey Ivanov <rbtz@dropbox.com>
@SaveTheRbtz
Copy link
Contributor Author

test failure is due to redirect_integration_test (#9145)

@lizan lizan merged commit be2829c into envoyproxy:master Dec 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants