Skip to content
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

sieve with bit-vec #12

Merged
merged 3 commits into from Mar 14, 2019
Merged

sieve with bit-vec #12

merged 3 commits into from Mar 14, 2019

Conversation

burdges
Copy link
Contributor

@burdges burdges commented Mar 14, 2019

I believe this improves sieve cache performance with the bit-vec crate, although I've some hiccup with cargo bench for testing this. This also fixes the confusing reference in #11

Can anyone tell me why this seperate binding was here?
improve reference to Chia network's `inkfish` implementation of
"Close to Uniform Prime Number GenerationWith Fewer Random Bits"
by Pierre-Alain Fouque and Mehdi Tibouchi
https://eprint.iacr.org/2011/481.pdf

Closes poanetwork#11
Copy link
Collaborator

@afck afck left a comment

Choose a reason for hiding this comment

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

Looks good to me!
Do you have any numbers on whether it makes a difference? Even without proper benchmarks, just using time, does it speed things up?

@afck afck requested a review from vkomenda March 14, 2019 14:30
@burdges
Copy link
Contributor Author

burdges commented Mar 14, 2019

Just fyi my benchmarks look like:

     Running /Users/jeff/Projects/PolakaDot/vdf/target/release/deps/classgroup_bench-101ad79d6fa635b8
Gnuplot not found, disabling plotting
thread 'main' panicked at 'bug in calling script', src/libcore/option.rs:1038:5
stack backtrace:
   0: std::sys::unix::backtrace::tracing::imp::unwind_backtrace
   1: std::sys_common::backtrace::_print
   2: std::panicking::default_hook::{{closure}}
   3: std::panicking::default_hook
   4: std::panicking::rust_panic_with_hook
   5: std::panicking::continue_panic_fmt
   6: rust_begin_unwind
   7: core::panicking::panic_fmt
   8: core::option::expect_failed
   9: classgroup_bench::main
  10: std::rt::lang_start::{{closure}}
  11: std::panicking::try::do_call
  12: __rust_maybe_catch_panic
  13: std::rt::lang_start_internal
  14: main
error: bench failed

@burdges
Copy link
Contributor Author

burdges commented Mar 14, 2019

Just running time cargo test create_discriminant shows no concrete improvements, so maybe worth fixing the benchmarks.

@burdges
Copy link
Contributor Author

burdges commented Mar 14, 2019

There was a BitVec in std way back https://doc.rust-lang.org/1.2.0/std/collections/struct.BitVec.html but never got stabilized due to limits on indexing https://github.com/Gankro/rfcs/blob/collections2/text/0000-collections-reform-part-2.md

@vkomenda
Copy link

Just fyi my benchmarks look like

Well, my benchmarks don't look like yours. 😄

$ ./bench.sh 123456
   Compiling clap v2.32.0
   Compiling criterion v0.2.10
   Compiling vdf v0.1.0 (/home/vk/src/poanetwork/vdf/vdf)
    Finished release [optimized] target(s) in 30.75s
     Running target/release/deps/classgroup_bench-992367e616934bab
square with seed 123456: 512                                                                             
                        time:   [16.155 us 16.186 us 16.223 us]
Found 8 outliers among 100 measurements (8.00%)
  3 (3.00%) high mild
  5 (5.00%) high severe

multiply with seed 123456: 512                                                                             
                        time:   [20.729 us 20.802 us 20.915 us]
Found 7 outliers among 100 measurements (7.00%)
  1 (1.00%) high mild
  6 (6.00%) high severe

square with seed 123456: 1024                                                                             
                        time:   [36.114 us 36.209 us 36.356 us]
Found 11 outliers among 100 measurements (11.00%)
  4 (4.00%) high mild
  7 (7.00%) high severe

multiply with seed 123456: 1024                                                                             
                        time:   [46.264 us 46.401 us 46.593 us]
Found 11 outliers among 100 measurements (11.00%)
  4 (4.00%) high mild
  7 (7.00%) high severe

square with seed 123456: 2048                                                                            
                        time:   [89.324 us 89.647 us 90.055 us]
Found 8 outliers among 100 measurements (8.00%)
  3 (3.00%) high mild
  5 (5.00%) high severe

multiply with seed 123456: 2048                                                                            
                        time:   [115.92 us 116.10 us 116.32 us]
Found 4 outliers among 100 measurements (4.00%)
  3 (3.00%) high mild
  1 (1.00%) high severe

@burdges
Copy link
Contributor Author

burdges commented Mar 14, 2019

Oddly this may even cause a regression: w3f@961caee

@burdges
Copy link
Contributor Author

burdges commented Mar 14, 2019

I see, cargo bench in the vdf directory does not work, but so far the bench.sh script seemingly works. Thanks!

Copy link

@vkomenda vkomenda left a comment

Choose a reason for hiding this comment

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

Looks good!

@vkomenda vkomenda merged commit c740ef3 into poanetwork:master Mar 14, 2019
@burdges
Copy link
Contributor Author

burdges commented Mar 14, 2019

Cool. I suppose this allocation must be being optimized away, which supports the regression in that other branch w3f@961caee for which I did not send a pull request. Anyways..

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.

None yet

3 participants