Minor optimization, prep work for more optimization #204

koverstreet · 2016-07-08T23:26:57Z

The patch "drop some dependencies on BigDigit's size" is the one I'd really like to get in.

cuviper · 2016-07-09T00:19:06Z

I'd prefer to see these as separate PRs, so we can keep focus separately. It may be that some parts are too intertwined to work separately, but so be it. Let's do it incrementally. And CI needs to pass, of course -- you have some rust-1.0 issues.

Mul with primitives is good, but I'll want the full complement. All ops, primitive either on left or right, and all variations of val/ref (including primitives by reference). More macros will probably help.
Shifting -- what's the performance like without unsafe? We have very minimal use of unsafe so far, all easily verified, and I'd really rather not open the door to big blocks of code like this.
Starting to isolating BigDigit is fine, but I'm going to be really hesitant to make bigger changes until we jump more fully into The dependencies of the bigint feature on rand and rustc_serialize should be optional. #151. These changes should show performance comparisons too, please.

koverstreet · 2016-07-09T01:47:05Z

Sounds good - this PR is now just the BigDigit changes, leaving the others for now. Added benchmark results.

cuviper · 2016-07-12T05:57:50Z

bigint/src/lib.rs

@@ -127,6 +127,7 @@ pub mod big_digit {

    // `DoubleBigDigit` size dependent
    pub const BITS: usize = 32;
+    pub use ::std::u32::MAX;


Let's not add any new pub items please. In a perfect world, I wouldn't even make big_digit public at all...
Just make it a top-level private name if you need to use it throughout the module.

This wasn't addressed in your update.

Whoops, fixed.

On Thu, Jul 14, 2016 at 2:12 PM, Josh Stone notifications@github.com
wrote:

In bigint/src/lib.rs
#204 (comment):

@@ -127,6 +127,7 @@ pub mod big_digit {

// `DoubleBigDigit` size dependent pub const BITS: usize = 32;

pub use ::std::u32::MAX;

This wasn't addressed in your update.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/rust-num/num/pull/204/files/4f92ac79ec2efeb675e24589bd00fdd1efd31906#r70893711,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AB5r7kY2E8pQHmYXpoLwKofSfCb1lCLFks5qVrRcgaJpZM4JIeee
.

cuviper · 2016-07-12T07:18:10Z

I think I've nitpicked enough for tonight...

We're still going to be a long way from freedom, as long as the bit-width is exposed in the public API, which is why I flagged the test changes. There really shouldn't be any changes to tests except to add more!

I'm guessing part of your reason is so you could test that a 64-bit BigDigit actually works. I think for now that will mean we need to divorce the public and private types. So the public BigDigit will remain 32-bit no matter what, until we make a breaking change to nuke it entirely, and the implementation should use only private types and values.

koverstreet · 2016-07-13T02:30:06Z

I think I fixed all your comments. I renamed ilog2 to fls, and I added a correct ilog2 which is now used by the str_radix code.

cuviper · 2016-07-14T22:12:18Z

bigint/src/lib.rs

+
+            for d in v.data.iter().rev() {
+                let digit_bits = (bits + big_digit::BITS - 1) %
+                    big_digit::BITS + 1;


Why are you adding BITS first? That should be a modular no-op. Only reason I can think is if bits were 0, to avoid subtraction underflow, but that should never be the case AFAICT.

bits shouldn't be 0 but IMO it's better and more readable to do it the way
that's correct in general, and post optimizer there's no difference anyways.

On Thu, Jul 14, 2016 at 2:12 PM, Josh Stone notifications@github.com
wrote:

In bigint/src/lib.rs
#204 (comment):

@@ -1349,6 +1356,47 @@ impl Integer for BigUint {
}
}

+fn high_bits_to_u64(v: &BigUint) -> u64 {

match v.data.len() {

0 => 0,

1 => v.data[0] as u64,

_ => {

let mut bits = v.bits();

let mut ret = 0u64;

let mut ret_bits = 0;

for d in v.data.iter().rev() {

let digit_bits = (bits + big_digit::BITS - 1) %

big_digit::BITS + 1;

Why are you adding BITS first? That should be a modular no-op. Only
reason I can think is if bits were 0, to avoid subtraction underflow, but
that should never be the case AFAICT.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/rust-num/num/pull/204/files/a1772f2a0d3e8fb7384a89322c93816b017fcc57#r70893665,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AB5r7mDoQNr2wrphE-LLhn4F55qAfqLeks5qVrRCgaJpZM4JIeee
.

Strictly speaking, it's not correct. It just trades a should-be-impossible subtraction error for a possible-but-unlikely addition overflow. And neither way gives a reasonable result, because bits==0 is a corner case where digit_bits should be 0, otherwise bits -= bits_want will break. It's not even more readable, because you had to wrap the line to fit the extra addition.

Hm. Actually, if adding to bits can overflow, bits itself can overflow -
it's a size_t, but size of an array in bits isn't guaranteed to fit in a
size_t.

On Jul 14, 2016 9:10 PM, "Josh Stone" notifications@github.com wrote:

In bigint/src/lib.rs
#204 (comment):

@@ -1349,6 +1356,47 @@ impl Integer for BigUint {
}
}

+fn high_bits_to_u64(v: &BigUint) -> u64 {

match v.data.len() {

0 => 0,

1 => v.data[0] as u64,

_ => {

let mut bits = v.bits();

let mut ret = 0u64;

let mut ret_bits = 0;

for d in v.data.iter().rev() {

let digit_bits = (bits + big_digit::BITS - 1) %

big_digit::BITS + 1;

Strictly speaking, it's not correct. It just trades a should-be-impossible
subtraction error for a possible-but-unlikely addition overflow. And
neither way gives a reasonable result, because bits==0 is a corner case
where digit_bits should be 0, otherwise bits -= bits_want will break. It's
not even more readable, because you had to wrap the line to fit the extra
addition.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/rust-num/num/pull/204/files/a1772f2a0d3e8fb7384a89322c93816b017fcc57#r70922274,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AB5r7mEtEkpH9qmvqF0B6TYOYBTKvh5Fks5qVxYpgaJpZM4JIeee
.

Yes. On 32-bit platforms, a BigUint that grows more than 512MB is going to have a bad time anywhere that's using ::bits(). One that's just on the edge could create the possible-but-unlikely addition overflow here. Meh.

That's kind of how I feel :)

I can go back to the subtract, I just want to get this patch in and get to
more interesting stuff...

On Thu, Jul 14, 2016 at 9:53 PM, Josh Stone notifications@github.com
wrote:

In bigint/src/lib.rs
#204 (comment):

@@ -1349,6 +1356,47 @@ impl Integer for BigUint {
}
}

+fn high_bits_to_u64(v: &BigUint) -> u64 {

match v.data.len() {

0 => 0,

1 => v.data[0] as u64,

_ => {

let mut bits = v.bits();

let mut ret = 0u64;

let mut ret_bits = 0;

for d in v.data.iter().rev() {

let digit_bits = (bits + big_digit::BITS - 1) %

big_digit::BITS + 1;

Yes. On 32-bit platforms, a BigUint that grows more than 512MB is going
to have a bad time anywhere that's using ::bits(). One that's just on the
edge could create the possible-but-unlikely addition overflow here. Meh.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/rust-num/num/pull/204/files/a1772f2a0d3e8fb7384a89322c93816b017fcc57#r70924708,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AB5r7kjuyGdzI_1w4gfKcARYpU5KQZxfks5qVyBOgaJpZM4JIeee
.

Before: test divide_0 ... bench: 1,011 ns/iter (+/- 184) test divide_1 ... bench: 18,535 ns/iter (+/- 770) test divide_2 ... bench: 990,467 ns/iter (+/- 91,980) test fac_to_string ... bench: 1,275 ns/iter (+/- 60) test factorial_100 ... bench: 6,453 ns/iter (+/- 101) test fib_100 ... bench: 1,142 ns/iter (+/- 99) test fib_1000 ... bench: 18,713 ns/iter (+/- 2,172) test fib_10000 ... bench: 1,197,965 ns/iter (+/- 21,178) test fib_to_string ... bench: 225 ns/iter (+/- 13) test from_str_radix_02 ... bench: 3,460 ns/iter (+/- 626) test from_str_radix_08 ... bench: 1,324 ns/iter (+/- 24) test from_str_radix_10 ... bench: 1,488 ns/iter (+/- 19) test from_str_radix_16 ... bench: 969 ns/iter (+/- 22) test from_str_radix_36 ... bench: 1,135 ns/iter (+/- 23) test hash ... bench: 102,126 ns/iter (+/- 1,016) test multiply_0 ... bench: 353 ns/iter (+/- 74) test multiply_1 ... bench: 31,006 ns/iter (+/- 679) test multiply_2 ... bench: 3,438,143 ns/iter (+/- 47,640) test pow_bench ... bench: 7,457,045 ns/iter (+/- 96,175) test shl ... bench: 5,627 ns/iter (+/- 121) test shr ... bench: 5,054 ns/iter (+/- 112) test to_str_radix_02 ... bench: 2,774 ns/iter (+/- 88) test to_str_radix_08 ... bench: 980 ns/iter (+/- 425) test to_str_radix_10 ... bench: 3,029 ns/iter (+/- 115) test to_str_radix_16 ... bench: 788 ns/iter (+/- 14) test to_str_radix_36 ... bench: 8,285 ns/iter (+/- 175) After: test divide_0 ... bench: 925 ns/iter (+/- 30) test divide_1 ... bench: 17,660 ns/iter (+/- 379) test divide_2 ... bench: 972,427 ns/iter (+/- 7,560) test fac_to_string ... bench: 1,260 ns/iter (+/- 36) test factorial_100 ... bench: 7,077 ns/iter (+/- 204) test fib_100 ... bench: 1,124 ns/iter (+/- 32) test fib_1000 ... bench: 18,475 ns/iter (+/- 166) test fib_10000 ... bench: 1,192,748 ns/iter (+/- 27,128) test fib_to_string ... bench: 228 ns/iter (+/- 10) test from_str_radix_02 ... bench: 3,379 ns/iter (+/- 74) test from_str_radix_08 ... bench: 1,355 ns/iter (+/- 24) test from_str_radix_10 ... bench: 1,470 ns/iter (+/- 20) test from_str_radix_16 ... bench: 958 ns/iter (+/- 239) test from_str_radix_36 ... bench: 1,137 ns/iter (+/- 19) test hash ... bench: 102,730 ns/iter (+/- 39,897) test multiply_0 ... bench: 351 ns/iter (+/- 15) test multiply_1 ... bench: 31,139 ns/iter (+/- 1,053) test multiply_2 ... bench: 3,464,509 ns/iter (+/- 124,235) test pow_bench ... bench: 7,448,428 ns/iter (+/- 326,903) test shl ... bench: 5,784 ns/iter (+/- 190) test shr ... bench: 4,820 ns/iter (+/- 63) test to_str_radix_02 ... bench: 2,757 ns/iter (+/- 33) test to_str_radix_08 ... bench: 989 ns/iter (+/- 67) test to_str_radix_10 ... bench: 3,045 ns/iter (+/- 70) test to_str_radix_16 ... bench: 787 ns/iter (+/- 24) test to_str_radix_36 ... bench: 8,257 ns/iter (+/- 117)/

cuviper · 2016-07-15T06:36:34Z

@homu r+ 8e0baec

homu · 2016-07-15T06:36:48Z

⚡ Test exempted - status

Minor optimization, prep work for more optimization The patch "drop some dependencies on BigDigit's size" is the one I'd really like to get in.

Derive Debug for Value

koverstreet force-pushed the master branch 3 times, most recently from 84b1e4b to c88c84a Compare July 9, 2016 01:36

koverstreet force-pushed the master branch from c88c84a to 4f92ac7 Compare July 12, 2016 00:35

cuviper reviewed Jul 12, 2016
View reviewed changes

koverstreet force-pushed the master branch from 4f92ac7 to b695a9f Compare July 13, 2016 02:26

koverstreet force-pushed the master branch from b695a9f to a1772f2 Compare July 13, 2016 06:32

cuviper reviewed Jul 14, 2016
View reviewed changes

koverstreet force-pushed the master branch from a1772f2 to 0e36cc5 Compare July 15, 2016 02:54

koverstreet force-pushed the master branch from 0e36cc5 to 8e0baec Compare July 15, 2016 05:56

homu merged commit 8e0baec into rust-num:master Jul 15, 2016

homu added a commit that referenced this pull request Jul 15, 2016

Auto merge of #204 - koverstreet:master, r=cuviper

78bad13

Minor optimization, prep work for more optimization The patch "drop some dependencies on BigDigit's size" is the one I'd really like to get in.

remexre pushed a commit to remexre/num that referenced this pull request Jun 1, 2017

Merge pull request rust-num#204 from serde-rs/debug

9f31764

Derive Debug for Value

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minor optimization, prep work for more optimization #204

Minor optimization, prep work for more optimization #204

koverstreet commented Jul 8, 2016

cuviper commented Jul 9, 2016

koverstreet commented Jul 9, 2016

cuviper Jul 12, 2016

cuviper Jul 14, 2016

koverstreet Jul 15, 2016

cuviper commented Jul 12, 2016

koverstreet commented Jul 13, 2016

cuviper Jul 14, 2016

koverstreet Jul 15, 2016

cuviper Jul 15, 2016

koverstreet Jul 15, 2016

cuviper Jul 15, 2016

koverstreet Jul 15, 2016

cuviper commented Jul 15, 2016

homu commented Jul 15, 2016

Minor optimization, prep work for more optimization #204

Minor optimization, prep work for more optimization #204

Conversation

koverstreet commented Jul 8, 2016

cuviper commented Jul 9, 2016

koverstreet commented Jul 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cuviper commented Jul 12, 2016

koverstreet commented Jul 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cuviper commented Jul 15, 2016

homu commented Jul 15, 2016