Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1413053002:
Change to use local Random object instead of global rand() in the RtcEventLog unit test. (Closed)
DescriptionChange to use local Random object instead of global rand() in the RtcEventLog unit test.
Removed Rand(int low, int high) since that function outputs results that are non-random and/or outside the interval if low is negative.
Added new Uniform(uint32_t, uint32_t) function to replace Rand(int low, int high).
Changed various unit tests to use the new functions.
BUG=
Committed: https://crrev.com/56b1128c8f14ea307994c27671e8884812b2704a
Cr-Commit-Position: refs/heads/master@{#10541}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Updated WebRTC unit tests to use the new functions in Random #
Total comments: 11
Patch Set 3 : Pass PRNG by pointer rather than by reference #Patch Set 4 : Renamed Random::Uniform to Random::Rand and made Rand<float> a template specialization #Patch Set 5 : Rebase #
Total comments: 4
Patch Set 6 : Comments from pbos #
Messages
Total messages: 34 (10 generated)
terelius@webrtc.org changed reviewers: + ivoc@webrtc.org, solenberg@webrtc.org, stefan@webrtc.org
This CL replaces the global rand() state with a local object as requested by solenberg@ in https://codereview.webrtc.org/1303713002/. (Please note that we never relied on always obtaining the same sequence of random number from the same seed, so the old unit test would still have passed even if some other thread modified the rand state.)
https://codereview.webrtc.org/1413053002/diff/1/webrtc/call/rtc_event_log_uni... File webrtc/call/rtc_event_log_unittest.cc (right): https://codereview.webrtc.org/1413053002/diff/1/webrtc/call/rtc_event_log_uni... webrtc/call/rtc_event_log_unittest.cc:310: csrcs.push_back(prng.Rand(0, 1000000000)); Duplicated a lot. Would it make sense to add a helper function on Random? template<typename T> T Rand() { return Rand(std::numeric_limits<T>::min(), std::numeric_limits<T>::max()); } then call site would use e.g. prng.Rand<uint16_t>() etc Could even make a specialization for bool: template<bool> bool Rand() { return Rand(0, 1) == 1; } Maybe overkill. WDYT?
Description was changed from ========== Change to use local Random object instead of global rand() in the RtcEventLog unit test. BUG= ========== to ========== Change to use local Random object instead of global rand() in the RtcEventLog unit test. Removed Rand(int low, int high) since that function outputs results that are non-random and/or outside the interval if low is negative. Added new Uniform(uint32_t, uint32_t) function to replace Rand(int low, int high). Changed various unit tests to use the new functions. BUG= ==========
https://codereview.webrtc.org/1413053002/diff/1/webrtc/call/rtc_event_log_uni... File webrtc/call/rtc_event_log_unittest.cc (right): https://codereview.webrtc.org/1413053002/diff/1/webrtc/call/rtc_event_log_uni... webrtc/call/rtc_event_log_unittest.cc:310: csrcs.push_back(prng.Rand(0, 1000000000)); On 2015/10/19 11:47:26, the sun wrote: > Duplicated a lot. Would it make sense to add a helper function on Random? > > template<typename T> T Rand() { > return Rand(std::numeric_limits<T>::min(), std::numeric_limits<T>::max()); > } > > then call site would use e.g. prng.Rand<uint16_t>() etc > > Could even make a specialization for bool: > > template<bool> bool Rand() { > return Rand(0, 1) == 1; > } > > Maybe overkill. WDYT? I was actually wishing for a function which returns a random bool, but I decided against it because the name "RandBool" was too ugly. The template specialization seems a lot less ad hoc. :)
Please make sure this builds with ./webrtc/build/gyp_webrtc -Denable_bwe_test_logging=1 as that code uses this prng. https://codereview.webrtc.org/1413053002/diff/20001/webrtc/call/rtc_event_log... File webrtc/call/rtc_event_log_unittest.cc (right): https://codereview.webrtc.org/1413053002/diff/20001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_unittest.cc:293: test::Random& prng) { test::Random*? https://codereview.webrtc.org/1413053002/diff/20001/webrtc/test/random.cc File webrtc/test/random.cc (right): https://codereview.webrtc.org/1413053002/diff/20001/webrtc/test/random.cc#new... webrtc/test/random.cc:30: } Should this be implemented using Uniform() instead?
https://codereview.webrtc.org/1413053002/diff/20001/webrtc/test/random.cc File webrtc/test/random.cc (right): https://codereview.webrtc.org/1413053002/diff/20001/webrtc/test/random.cc#new... webrtc/test/random.cc:30: } On 2015/10/22 09:41:22, stefan-webrtc (holmer) wrote: > Should this be implemented using Uniform() instead? Maybe the a_ ^= b_; b_ += a_; part could be put in an internal helper uint32_t Next()? However, I'm fine duplicating the code as well. Your call. https://codereview.webrtc.org/1413053002/diff/20001/webrtc/test/random.cc#new... webrtc/test/random.cc:35: result = result >> 32; Nice! https://codereview.webrtc.org/1413053002/diff/20001/webrtc/test/random.h File webrtc/test/random.h (right): https://codereview.webrtc.org/1413053002/diff/20001/webrtc/test/random.h#newc... webrtc/test/random.h:31: T Rand() { This is very pretty! Why are some functions called Rand() and some Uniform()? Should the float version also be a template specialization or is that just adding cumbersome syntax?
https://codereview.webrtc.org/1413053002/diff/20001/webrtc/call/rtc_event_log... File webrtc/call/rtc_event_log_unittest.cc (right): https://codereview.webrtc.org/1413053002/diff/20001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_unittest.cc:293: test::Random& prng) { On 2015/10/22 09:41:22, stefan-webrtc (holmer) wrote: > test::Random*? Done. https://codereview.webrtc.org/1413053002/diff/20001/webrtc/test/random.cc File webrtc/test/random.cc (right): https://codereview.webrtc.org/1413053002/diff/20001/webrtc/test/random.cc#new... webrtc/test/random.cc:30: } On 2015/10/22 09:41:22, stefan-webrtc (holmer) wrote: > Should this be implemented using Uniform() instead? Using Uniform(MAX_INT) would give the exact same result as the current code, but requires 64-bit arithmetic. I think it is better to keep it as it is for now. https://codereview.webrtc.org/1413053002/diff/20001/webrtc/test/random.cc#new... webrtc/test/random.cc:30: } On 2015/10/22 12:30:38, the sun wrote: > On 2015/10/22 09:41:22, stefan-webrtc (holmer) wrote: > > Should this be implemented using Uniform() instead? > > Maybe the a_ ^= b_; b_ += a_; part could be put in an internal helper uint32_t > Next()? > > However, I'm fine duplicating the code as well. Your call. Considering it is only 2 simple lines I think it is fine to duplicate them here. When I rewrite the PRNG to use an xorshift* algorithm I'll make sure to add a helper function to advance the state though. https://codereview.webrtc.org/1413053002/diff/20001/webrtc/test/random.h File webrtc/test/random.h (right): https://codereview.webrtc.org/1413053002/diff/20001/webrtc/test/random.h#newc... webrtc/test/random.h:31: T Rand() { On 2015/10/22 12:30:38, the sun wrote: > This is very pretty! > > Why are some functions called Rand() and some Uniform()? > > Should the float version also be a template specialization or is that just > adding cumbersome syntax? I considered both ideas, but couldn't make up my mind. :) My (weak) argument for doing it this way was that the functions which takes a distribution parameter should be named after that distribution, e.g Uniform. I kept Rand in analogy with the C rand() function when you want a random element but haven't explicitly specified a distribution. I don't have any strong opinions. What would you prefer?
https://codereview.webrtc.org/1413053002/diff/20001/webrtc/test/random.h File webrtc/test/random.h (right): https://codereview.webrtc.org/1413053002/diff/20001/webrtc/test/random.h#newc... webrtc/test/random.h:31: T Rand() { On 2015/10/26 18:18:41, terelius wrote: > On 2015/10/22 12:30:38, the sun wrote: > > This is very pretty! > > > > Why are some functions called Rand() and some Uniform()? > > > > Should the float version also be a template specialization or is that just > > adding cumbersome syntax? > > I considered both ideas, but couldn't make up my mind. :) > > My (weak) argument for doing it this way was that the functions which takes a > distribution parameter should be named after that distribution, e.g Uniform. I > kept Rand in analogy with the C rand() function when you want a random element > but haven't explicitly specified a distribution. > > I don't have any strong opinions. What would you prefer? Ok, I understand your argument. I think we should stick to Rand() though (leave Gaussian and Exponential as they are), but rename the Uniform() functions. I don't have super solid reasons either: 1. The distribution is a property of the implementation and doesn't differ between the Rand() and Uniform() functions. 2. The different naming will likely cause more confusion than it is helping. I imagine users commonly will look for a function to return a PRN within a range, of a certain type and so, rather than a certain distribution. Also, leave the float impl as it is now.
https://codereview.webrtc.org/1413053002/diff/20001/webrtc/test/random.h File webrtc/test/random.h (right): https://codereview.webrtc.org/1413053002/diff/20001/webrtc/test/random.h#newc... webrtc/test/random.h:31: T Rand() { On 2015/10/27 09:03:12, the sun wrote: > On 2015/10/26 18:18:41, terelius wrote: > > On 2015/10/22 12:30:38, the sun wrote: > > > This is very pretty! > > > > > > Why are some functions called Rand() and some Uniform()? > > > > > > Should the float version also be a template specialization or is that just > > > adding cumbersome syntax? > > > > I considered both ideas, but couldn't make up my mind. :) > > > > My (weak) argument for doing it this way was that the functions which takes a > > distribution parameter should be named after that distribution, e.g Uniform. I > > kept Rand in analogy with the C rand() function when you want a random element > > but haven't explicitly specified a distribution. > > > > I don't have any strong opinions. What would you prefer? > > Ok, I understand your argument. I think we should stick to Rand() though (leave > Gaussian and Exponential as they are), but rename the Uniform() functions. I > don't have super solid reasons either: > > 1. The distribution is a property of the implementation and doesn't differ > between the Rand() and Uniform() functions. > 2. The different naming will likely cause more confusion than it is helping. I > imagine users commonly will look for a function to return a PRN within a range, > of a certain type and so, rather than a certain distribution. > > Also, leave the float impl as it is now. But isn't it a bit weird (and error prone) to overload Rand such that it computes something completely differently when called with a parameter compared to no parameter? For example: int i = prng.Rand(99) // Works. Sets i to a random number in [0,99]. int i = prng.Rand() // Compiles and runs without errors, but i=0.
lgtm from my side.
On 2015/10/29 12:54:04, terelius wrote: > https://codereview.webrtc.org/1413053002/diff/20001/webrtc/test/random.h > File webrtc/test/random.h (right): > > https://codereview.webrtc.org/1413053002/diff/20001/webrtc/test/random.h#newc... > webrtc/test/random.h:31: T Rand() { > On 2015/10/27 09:03:12, the sun wrote: > > On 2015/10/26 18:18:41, terelius wrote: > > > On 2015/10/22 12:30:38, the sun wrote: > > > > This is very pretty! > > > > > > > > Why are some functions called Rand() and some Uniform()? > > > > > > > > Should the float version also be a template specialization or is that just > > > > adding cumbersome syntax? > > > > > > I considered both ideas, but couldn't make up my mind. :) > > > > > > My (weak) argument for doing it this way was that the functions which takes > a > > > distribution parameter should be named after that distribution, e.g Uniform. > I > > > kept Rand in analogy with the C rand() function when you want a random > element > > > but haven't explicitly specified a distribution. > > > > > > I don't have any strong opinions. What would you prefer? > > > > Ok, I understand your argument. I think we should stick to Rand() though > (leave > > Gaussian and Exponential as they are), but rename the Uniform() functions. I > > don't have super solid reasons either: > > > > 1. The distribution is a property of the implementation and doesn't differ > > between the Rand() and Uniform() functions. > > 2. The different naming will likely cause more confusion than it is helping. I > > imagine users commonly will look for a function to return a PRN within a > range, > > of a certain type and so, rather than a certain distribution. > > > > Also, leave the float impl as it is now. > > But isn't it a bit weird (and error prone) to overload Rand such that it > computes something completely differently when called with a parameter compared > to no parameter? > > For example: > int i = prng.Rand(99) // Works. Sets i to a random number in [0,99]. > int i = prng.Rand() // Compiles and runs without errors, but i=0. Isn't that more an argument to make the float version a template specialization?
On 2015/10/29 13:05:59, the sun wrote: > On 2015/10/29 12:54:04, terelius wrote: > > https://codereview.webrtc.org/1413053002/diff/20001/webrtc/test/random.h > > File webrtc/test/random.h (right): > > > > > https://codereview.webrtc.org/1413053002/diff/20001/webrtc/test/random.h#newc... > > webrtc/test/random.h:31: T Rand() { > > On 2015/10/27 09:03:12, the sun wrote: > > > On 2015/10/26 18:18:41, terelius wrote: > > > > On 2015/10/22 12:30:38, the sun wrote: > > > > > This is very pretty! > > > > > > > > > > Why are some functions called Rand() and some Uniform()? > > > > > > > > > > Should the float version also be a template specialization or is that > just > > > > > adding cumbersome syntax? > > > > > > > > I considered both ideas, but couldn't make up my mind. :) > > > > > > > > My (weak) argument for doing it this way was that the functions which > takes > > a > > > > distribution parameter should be named after that distribution, e.g > Uniform. > > I > > > > kept Rand in analogy with the C rand() function when you want a random > > element > > > > but haven't explicitly specified a distribution. > > > > > > > > I don't have any strong opinions. What would you prefer? > > > > > > Ok, I understand your argument. I think we should stick to Rand() though > > (leave > > > Gaussian and Exponential as they are), but rename the Uniform() functions. I > > > don't have super solid reasons either: > > > > > > 1. The distribution is a property of the implementation and doesn't differ > > > between the Rand() and Uniform() functions. > > > 2. The different naming will likely cause more confusion than it is helping. > I > > > imagine users commonly will look for a function to return a PRN within a > > range, > > > of a certain type and so, rather than a certain distribution. > > > > > > Also, leave the float impl as it is now. > > > > But isn't it a bit weird (and error prone) to overload Rand such that it > > computes something completely differently when called with a parameter > compared > > to no parameter? > > > > For example: > > int i = prng.Rand(99) // Works. Sets i to a random number in [0,99]. > > int i = prng.Rand() // Compiles and runs without errors, but i=0. > > Isn't that more an argument to make the float version a template specialization? Possibly. So your suggestion is Rand() -> Rand<float>() and Uniform(...) -> Rand(...)?
On 2015/10/29 13:10:32, terelius wrote: > On 2015/10/29 13:05:59, the sun wrote: > > On 2015/10/29 12:54:04, terelius wrote: > > > https://codereview.webrtc.org/1413053002/diff/20001/webrtc/test/random.h > > > File webrtc/test/random.h (right): > > > > > > > > > https://codereview.webrtc.org/1413053002/diff/20001/webrtc/test/random.h#newc... > > > webrtc/test/random.h:31: T Rand() { > > > On 2015/10/27 09:03:12, the sun wrote: > > > > On 2015/10/26 18:18:41, terelius wrote: > > > > > On 2015/10/22 12:30:38, the sun wrote: > > > > > > This is very pretty! > > > > > > > > > > > > Why are some functions called Rand() and some Uniform()? > > > > > > > > > > > > Should the float version also be a template specialization or is that > > just > > > > > > adding cumbersome syntax? > > > > > > > > > > I considered both ideas, but couldn't make up my mind. :) > > > > > > > > > > My (weak) argument for doing it this way was that the functions which > > takes > > > a > > > > > distribution parameter should be named after that distribution, e.g > > Uniform. > > > I > > > > > kept Rand in analogy with the C rand() function when you want a random > > > element > > > > > but haven't explicitly specified a distribution. > > > > > > > > > > I don't have any strong opinions. What would you prefer? > > > > > > > > Ok, I understand your argument. I think we should stick to Rand() though > > > (leave > > > > Gaussian and Exponential as they are), but rename the Uniform() functions. > I > > > > don't have super solid reasons either: > > > > > > > > 1. The distribution is a property of the implementation and doesn't differ > > > > between the Rand() and Uniform() functions. > > > > 2. The different naming will likely cause more confusion than it is > helping. > > I > > > > imagine users commonly will look for a function to return a PRN within a > > > range, > > > > of a certain type and so, rather than a certain distribution. > > > > > > > > Also, leave the float impl as it is now. > > > > > > But isn't it a bit weird (and error prone) to overload Rand such that it > > > computes something completely differently when called with a parameter > > compared > > > to no parameter? > > > > > > For example: > > > int i = prng.Rand(99) // Works. Sets i to a random number in [0,99]. > > > int i = prng.Rand() // Compiles and runs without errors, but i=0. > > > > Isn't that more an argument to make the float version a template > specialization? > > Possibly. So your suggestion is Rand() -> Rand<float>() and Uniform(...) -> > Rand(...)? Yes.
On 2015/10/29 13:31:05, the sun wrote: > On 2015/10/29 13:10:32, terelius wrote: > > On 2015/10/29 13:05:59, the sun wrote: > > > On 2015/10/29 12:54:04, terelius wrote: > > > > https://codereview.webrtc.org/1413053002/diff/20001/webrtc/test/random.h > > > > File webrtc/test/random.h (right): > > > > > > > > > > > > > > https://codereview.webrtc.org/1413053002/diff/20001/webrtc/test/random.h#newc... > > > > webrtc/test/random.h:31: T Rand() { > > > > On 2015/10/27 09:03:12, the sun wrote: > > > > > On 2015/10/26 18:18:41, terelius wrote: > > > > > > On 2015/10/22 12:30:38, the sun wrote: > > > > > > > This is very pretty! > > > > > > > > > > > > > > Why are some functions called Rand() and some Uniform()? > > > > > > > > > > > > > > Should the float version also be a template specialization or is > that > > > just > > > > > > > adding cumbersome syntax? > > > > > > > > > > > > I considered both ideas, but couldn't make up my mind. :) > > > > > > > > > > > > My (weak) argument for doing it this way was that the functions which > > > takes > > > > a > > > > > > distribution parameter should be named after that distribution, e.g > > > Uniform. > > > > I > > > > > > kept Rand in analogy with the C rand() function when you want a random > > > > element > > > > > > but haven't explicitly specified a distribution. > > > > > > > > > > > > I don't have any strong opinions. What would you prefer? > > > > > > > > > > Ok, I understand your argument. I think we should stick to Rand() though > > > > (leave > > > > > Gaussian and Exponential as they are), but rename the Uniform() > functions. > > I > > > > > don't have super solid reasons either: > > > > > > > > > > 1. The distribution is a property of the implementation and doesn't > differ > > > > > between the Rand() and Uniform() functions. > > > > > 2. The different naming will likely cause more confusion than it is > > helping. > > > I > > > > > imagine users commonly will look for a function to return a PRN within a > > > > range, > > > > > of a certain type and so, rather than a certain distribution. > > > > > > > > > > Also, leave the float impl as it is now. > > > > > > > > But isn't it a bit weird (and error prone) to overload Rand such that it > > > > computes something completely differently when called with a parameter > > > compared > > > > to no parameter? > > > > > > > > For example: > > > > int i = prng.Rand(99) // Works. Sets i to a random number in [0,99]. > > > > int i = prng.Rand() // Compiles and runs without errors, but i=0. > > > > > > Isn't that more an argument to make the float version a template > > specialization? > > > > Possibly. So your suggestion is Rand() -> Rand<float>() and Uniform(...) -> > > Rand(...)? > > Yes. Done
Nice! LGTM
Nice, LGTM.
The CQ bit was checked by terelius@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org, solenberg@webrtc.org, ivoc@webrtc.org Link to the patchset: https://codereview.webrtc.org/1413053002/#ps80001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1413053002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1413053002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/1583)
terelius@webrtc.org changed reviewers: + pbos@chromium.org
terelius@webrtc.org changed reviewers: + pbos@webrtc.org - pbos@chromium.org
Peter, could you review random.* please?
lgtm with nits fixed https://codereview.webrtc.org/1413053002/diff/80001/webrtc/test/random.cc File webrtc/test/random.cc (right): https://codereview.webrtc.org/1413053002/diff/80001/webrtc/test/random.cc#new... webrtc/test/random.cc:26: result = b_ * (result + 1); static_cast<uint64_t>(t) instead of result, that makes the upcast explicit https://codereview.webrtc.org/1413053002/diff/80001/webrtc/test/random.cc#new... webrtc/test/random.cc:27: result = result >> 32; result >>= 32;
https://codereview.webrtc.org/1413053002/diff/80001/webrtc/test/random.cc File webrtc/test/random.cc (right): https://codereview.webrtc.org/1413053002/diff/80001/webrtc/test/random.cc#new... webrtc/test/random.cc:26: result = b_ * (result + 1); On 2015/11/06 10:35:21, pbos-webrtc wrote: > static_cast<uint64_t>(t) instead of result, that makes the upcast explicit Done. https://codereview.webrtc.org/1413053002/diff/80001/webrtc/test/random.cc#new... webrtc/test/random.cc:27: result = result >> 32; On 2015/11/06 10:35:21, pbos-webrtc wrote: > result >>= 32; Done.
The CQ bit was checked by terelius@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1413053002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1413053002/80001
The CQ bit was checked by terelius@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org, ivoc@webrtc.org, pbos@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/1413053002/#ps100001 (title: "Comments from pbos")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1413053002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1413053002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/56b1128c8f14ea307994c27671e8884812b2704a Cr-Commit-Position: refs/heads/master@{#10541} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
