Skip to content

runtime: added standard library compatibility to RandomGenerator #9053

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.

Sign up for GitHub

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

Merged
lizan merged 2 commits into from
Dec 2, 2019
Merged

runtime: added standard library compatibility to RandomGenerator #9053

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1b95684
runtime: added standard library compatibility to RandomGenerator
SaveTheRbtz Nov 16, 2019
170b95e
clang-tidy: add lint
SaveTheRbtz Nov 26, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 22 additions & 1 deletion include/envoy/runtime/runtime.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#pragma once

#include <cstdint>
#include <limits>
#include <memory>
#include <string>
#include <unordered_map>
Expand Down Expand Up @@ -30,10 +31,30 @@ class RandomGenerator {
public:
virtual ~RandomGenerator() = default;

using result_type = uint64_t; // NOLINT(readability-identifier-naming)

/**
* @return uint64_t a new random number.
*/
virtual uint64_t random() PURE;
virtual result_type random() PURE;

/*
* @return the smallest value that `operator()` may return. The value is
* strictly less than `max()`.
*/
constexpr static result_type min() noexcept { return std::numeric_limits<result_type>::min(); };

/*
* @return the largest value that `operator()` may return. The value is
* strictly greater than `min()`.
*/
constexpr static result_type max() noexcept { return std::numeric_limits<result_type>::max(); };

/*
* @return a value in the closed interval `[min(), max()]`. Has amortized
* constant complexity.
*/
result_type operator()() { return result_type(random()); };

/**
* @return std::string containing uuid4 of 36 char length.
Expand Down
17 changes: 17 additions & 0 deletions test/common/runtime/runtime_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,23 @@ TEST(Random, SanityCheckOfUniquenessRandom) {
EXPECT_EQ(num_of_results, results.size());
}

TEST(Random, SanityCheckOfStdLibRandom) {
Runtime::RandomGeneratorImpl random;

static const auto num_of_items = 100;
std::vector<uint64_t> v(num_of_items);
std::iota(v.begin(), v.end(), 0);

static const auto num_of_checks = 10000;
for (size_t i = 0; i < num_of_checks; ++i) {
const auto prev = v;
std::shuffle(v.begin(), v.end(), random);
Copy link
Contributor

@dnoe dnoe Nov 22, 2019

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

@SaveTheRbtz SaveTheRbtz Nov 23, 2019

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.

EXPECT_EQ(v.size(), prev.size());
EXPECT_NE(v, prev);
EXPECT_FALSE(std::is_sorted(v.begin(), v.end()));
Copy link
Contributor

@dnoe dnoe Nov 22, 2019

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

@SaveTheRbtz SaveTheRbtz Nov 23, 2019

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

}
}

TEST(UUID, CheckLengthOfUUID) {
RandomGeneratorImpl random;

Expand Down