-
Notifications
You must be signed in to change notification settings - Fork 5
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
STR-985: Make ArbitraryGenerator
generate more entropy on the fly
#674
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
@@ Coverage Diff @@
## main #674 +/- ##
==========================================
+ Coverage 54.68% 54.69% +0.01%
==========================================
Files 318 318
Lines 34047 34055 +8
==========================================
+ Hits 18619 18627 +8
Misses 15428 15428
|
Commit: 60a10d1 SP1 Performance Test Results
|
The way the ticket was written, we can keep the comfortable buffer sizes (or even increase the default), and it would simply only generate the precise amount of randomness requested when asked for it.
^This is really the right way to do it, which I should have done when I reworked it last. Sure we can keep |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- One thing that crossed my mind is that if we want dynamic size estimation, we could probably use
std::mem::size_of::<T>()
. But this wouldn't work for structs that contain heap allocated items likeVec
. I really wish there was some way around this, because otherwise we have to try hard to estimate the size of the resulting object. - Another thing is that why are we opting for exact sizes instead of bit operations(like
1 << 12
) while usingnew_with_size()
. The latter seems easier to reason about just by looking at it and we can also easily control how many bits/bytes we are allocating.
@delbonis so the solution is doing the reverse of what I did? |
@bewakes I agree it would be better if it worked like that. But the @storopoli It's still good practice to constrain it where it's known to be safe so we use less memory. But this would be a more complete solution. |
Description
The default was 65kb, which was even called for
u64
, e.g.BitcoinAmount
.So let's make
ArbitraryGenerator
reasonable where the default is 1kb and callingnew_with_size()
for bigger sizes.Type of Change
Checklist
Related Issues
STR-985