Changes in ArbitraryGenerator #502
Conversation
- Used 128 bytes of default buffer size vs 2^24 bytes buffer size earlier. - Used thread_rng to generate random number in each call as compared to using slices from pre-initialized huge buffer - Updated unit tests accordingly to work with new changes.
Codecov ReportAll modified and coverable lines are covered by tests ✅
@@ Coverage Diff @@
## main #502 +/- ##
==========================================
+ Coverage 56.45% 56.48% +0.03%
==========================================
Files 273 275 +2
Lines 29079 29190 +111
==========================================
+ Hits 16416 16489 +73
- Misses 12663 12701 +38
|
| pub fn new() -> Self { | ||
| Self::new_with_size(ARB_GEN_LEN) | ||
| ArbitraryGenerator { | ||
| rng: thread_rng(), | ||
| buf: vec![0u8; ARB_GEN_LEN], | ||
| } | ||
| } | ||
|
|
||
| pub fn new_with_size(n: usize) -> Self { | ||
| let mut buf = vec![0; n]; | ||
| OsRng.fill_bytes(&mut buf); // 128 wasn't enough | ||
| let off = AtomicUsize::new(0); | ||
| ArbitraryGenerator { buf, off } | ||
| pub fn new_with_size(s: usize) -> Self { | ||
| ArbitraryGenerator { | ||
| rng: thread_rng(), | ||
| buf: vec![0u8; s], | ||
| } | ||
| } | ||
|
|
||
| pub fn generate<'a, T: Arbitrary<'a> + Clone>(&'a self) -> T { | ||
| // Doing hacky atomics to make this actually be reusable, this is pretty bad. | ||
| let off = self.off.load(Ordering::Relaxed); | ||
| let mut u = Unstructured::new(&self.buf[off..]); | ||
| let prev_off = u.len(); | ||
| let inst = T::arbitrary(&mut u).expect("failed to generate arbitrary instance"); | ||
| let additional_off = prev_off - u.len(); | ||
| self.off.store(off + additional_off, Ordering::Relaxed); | ||
| inst | ||
| pub fn generate<'a, T: Arbitrary<'a> + Clone>(&'a mut self) -> T { | ||
| self.rng.fill_bytes(&mut self.buf); | ||
| let mut u = Unstructured::new(&self.buf); | ||
| T::arbitrary(&mut u).expect("Failed to generate arbitrary instance") |
There was a problem hiding this comment.
The way this is written kinda makes having the random buf be a field pointless. Since we sample new randomness on each call it might as well just be a local and then we can just have this take &self. Also what happens if we consume more than the length of randomness in a call to generate? I'm not sure the best design approach here since it'd be nice to be able to make it deterministic if desired.
Also should verify if it makes sense to include the rng as a field as opposed to just calling thread_rng() on every invocation. I'm not sure if there's a practical difference in terms of speed/determinism there.
There was a problem hiding this comment.
I think the buf field is required because of lifetime issues. The random buf is generated inside the method which can't leave the method scope.
I don't really like having mut everywhere though. Maybe a RefCell can be used for buffer(and for rng?), which should be fine since we probably won't be using a generator instance across multiple threads. What are your thoughts on this?
There was a problem hiding this comment.
The way this is written kinda makes having the random buf be a field pointless.
The reason for having buf field is due to lifetime requirement, like @bewakes said.
Since we sample new randomness on each call it might as well just be a local and then we can just have this take
&self.
We needed &mut due to having to update randomness on every call.
Also what happens if we consume more than the length of randomness in a call to
generate? I'm not sure the best design approach here since it'd be nice to be able to make it deterministic if desired.
It's a good concern, but I think having new_with_size() serves that purpose.
Also should verify if it makes sense to include the rng as a field as opposed to just calling
thread_rng()on every invocation. I'm not sure if there's a practical difference in terms of speed/determinism there.
Both approaches definitely work, and don't see either way to be an issue.
There was a problem hiding this comment.
I think the buf field is required because of lifetime issues. The random buf is generated inside the method which can't leave the method scope.
That's what I observed too.
I don't really like having
muteverywhere though. Maybe aRefCellcan be used for buffer(and for rng?), which should be fine since we probably won't be using a generator instance across multiple threads. What are your thoughts on this?
Yeah, initially I did not like the idea of having to change multiple files to pass mutable arbitrary generator but due to lifetime requirement using RefCell was not helpful there.
There was a problem hiding this comment.
Oh christ I'm realizing how this api kinda sucks. I didn't see that Arbitrary has a lifetime param.
Taking &mut self means I think you can have multiple invocations now can you? Since you'd overwrite the buffer that we have a hypothetical ref to in the value returned in the previous invocation.
Is this an issue that we can deal with by generating the randomness upfront?
There was a problem hiding this comment.
I think you can have multiple invocations now can you?
Multiple arb.generate()? Yes.
There was a problem hiding this comment.
Sorry, I meant can't. Since you'd need to take an exclusive borrow while there was a shared borrow active.
Description
Type of Change
Notes to Reviewers
Checklist
Related Issues
STR-551