Port over relevant changes from bigint #25

dvdplm · 2018-08-10T12:19:57Z

The bigint crate is going away but there is valuable code there that is ported over in this PR.

During the recent extraction of crates from parity-ethereum and primitives, the uint crate does not instantiate any types, leaving that for consuming code to take care of (e.g. ethereum-types or substrate/primitives). This means that some code in bigint cannot be ported to uint but should rather be checked against what ethereum-types has today. Examples of this is the impl U256 {…}, the serde serialization stuff and the many conversion impls in bigint.

The bigint crate adds a mod_inverse function to U256. I think this is the only feature not present in uint. If it turns out we need this, it should probably be added to ethereum-types.

The repos have diverged significantly so this port has been made manually. I am sure I have missed things, so thorough review by the bigint committers would be very useful.

Performance

The uint crate did not have any benchmarks so this PR copies over the ones from bigint. The results are odd: uint is much faster across the board. This was unexpected and makes me suspect I did something wrong.

Results for uint:

running 12 tests
test u128_mul       ... bench:      10,466 ns/iter (+/- 649)
test u256_add       ... bench:       5,205 ns/iter (+/- 313)
test u256_from_be   ... bench:           0 ns/iter (+/- 0)
test u256_from_le   ... bench:           4 ns/iter (+/- 0)
test u256_full_mul  ... bench:      39,983 ns/iter (+/- 2,418)
test u256_mul       ... bench:      20,673 ns/iter (+/- 1,583)
test u256_mul_small ... bench:         144 ns/iter (+/- 7)
test u256_sub       ... bench:       5,248 ns/iter (+/- 370)
test u512_add       ... bench:      22,656 ns/iter (+/- 1,158)
test u512_mul       ... bench:      49,118 ns/iter (+/- 2,795)
test u512_mul_small ... bench:         587 ns/iter (+/- 37)
test u512_sub       ... bench:      24,038 ns/iter (+/- 1,502)

test result: ok. 0 passed; 0 failed; 0 ignored; 12 measured; 0 filtered out

Results for bigint:

running 12 tests
test u128_mul       ... bench:      15,753 ns/iter (+/- 921)
test u256_add       ... bench:      21,017 ns/iter (+/- 1,666)
test u256_from_be   ... bench:           4 ns/iter (+/- 0)
test u256_from_le   ... bench:           6 ns/iter (+/- 0)
test u256_full_mul  ... bench:      40,166 ns/iter (+/- 2,646)
test u256_mul       ... bench:      67,205 ns/iter (+/- 4,982)
test u256_mul_small ... bench:         489 ns/iter (+/- 47)
test u256_sub       ... bench:      26,734 ns/iter (+/- 3,208)
test u512_add       ... bench:      22,578 ns/iter (+/- 1,352)
test u512_mul       ... bench:     320,132 ns/iter (+/- 18,449)
test u512_mul_small ... bench:       3,700 ns/iter (+/- 314)
test u512_sub       ... bench:      23,576 ns/iter (+/- 1,819)

…in no_std

eira-fransham

The only issue is that you unnecessarily create a ~1kb buffer when converting to a string

eira-fransham · 2018-08-13T09:36:23Z

uint/src/uint.rs

@@ -1219,16 +1195,24 @@ macro_rules! impl_std_for_uint {
 					return write!(f, "0");
 				}

-				let mut s = String::new();
+				let mut buf = [0_u8; $n_words*20];


Why $n_words * 20? Surely it's ceil(log10(2^($n_words - 1)))? That's 154 for U512 but this code would create a buffer of size 1280 (way too big).

This change came from https://github.com/paritytech/bigint/blob/master/src/uint.rs#L927 (part of PR #43) – not sure what their thinking was there, maybe it was making space for a somewhat long debug string (which doesn't make sense).
Should I just go ahead and replace it?

Yes please!

Wait a sec, I think you're confusing uint with fixed-hash: here the construct_uint! macro is called with the words not the number of bytes, so $n_words here is 8 => 8 * 10 == 160 which is fine I think.

The construct_hash! macro takes number of bytes, which is more logical imo. Quite confusing.

eira-fransham · 2018-08-13T09:40:46Z

uint/src/uint.rs

 #[macro_export]
 #[doc(hidden)]
-macro_rules! impl_std_for_uint {
-	($name: ident, $n_words: tt) => {}
+macro_rules! impl_std_for_uint_internals {


Why change the name of this macro? If it's used by another macro then you should use a private pattern of that macro rather than a doc(hidden) macro.

NikVolf · 2018-08-13T12:07:35Z

why do we need the same code in 2 places anyway?

dvdplm · 2018-08-13T13:31:25Z

why do we need the same code in 2 places anyway?

Not sure I follow, can you elaborate? The idea here is to:

bring useful changes from bigint into uint and then
stop using bigint

NikVolf · 2018-08-13T13:58:38Z

uint/Cargo.toml

@@ -4,27 +4,23 @@ homepage = "http://parity.io"
 repository = "https://github.com/paritytech/primitives"


btw, repository needs an update?

NikVolf · 2018-08-13T14:05:27Z

You import code from MIT/Apache repository, btw

under GPL license while external contributors agreed under MIT/Apache contribution

https://github.com/paritytech/bigint#contribution

Use --release to run tests

code is correct

Pzixel · 2018-08-13T15:09:45Z

Hey guys. I contributed a bit but I don't mind if you copy my code under diffirent license.

About my changes in no_std format: $n_words*20 is the smallest buffer you could use to format any uint of size $n_words losslessly. It's not just a round number or something. I don't recall exactly why you cannot use $n_words*16 (which naively seems to be the correct smallest buffer size), but you can't.

dvdplm · 2018-08-13T16:04:58Z

@Pzixel your code is correct, we were mistaken. :) And thank you for your contribution and openness to sort out the licensing stuff. Much appreciated!

Pzixel · 2018-08-13T16:42:17Z

Great work!

I'd also suggest to review this discussion about implementing panicing From trait: paritytech/bigint#43 (comment)

In a nutshell: rust documentation clearly says that From should not fail (and panic is obviosly a fail), when guys think it's ok becase you have a 'static constraint on it. You probably want t revisit with decision before merging into master.

eira-fransham · 2018-08-14T11:43:00Z

@Pzixel According to this simple Rust program:

fn main() {
    for &i in &[128, 256, 512] {
        println!("{}, {}", (i / 32) * 10, 2f64.powi(i - 1).log10().ceil());
    }
}

// Prints
// 40, 39
// 80, 77
// 160, 154

You could get away with doing $n_words * 10 and it'd still work.

eira-fransham · 2018-08-14T11:46:26Z

I think that the best way to handle the bad From impl is to either do U256::try_from("123123").unwrap() or have an inherent fn that takes a &str of any lifetime with a doc comment saying that it may panic.

Pzixel · 2018-08-14T12:20:47Z

@Vurich

You could get away with doing $n_words * 10 and it'd still work.

you can easily change the code and see that tests don't pass 🙂

eira-fransham · 2018-08-14T12:26:36Z

What error? It seems like you should get an OOB index and not a UTF8 error,

Pzixel · 2018-08-14T12:39:25Z

@Vurich yep, this is why I edited my post. But you probably got an old version in email notification. Nevermind. Just check it yorself if you wish, it will grant better understanding and I will be calm that I didn't lie :)

debris

lgtm, let's just merge it :)

dvdplm · 2018-08-14T12:56:12Z

Using $n_words * 10 does indeed fail.

Pzixel · 2018-08-14T12:56:33Z

I think that the best way to handle the bad From impl is to either do U256::try_from("123123").unwrap() or have an inherent fn that takes a &str of any lifetime with a doc comment saying that it may panic.

there is already Parse so you don't need any try_from, just parse :) See paritytech/bigint@9183976#diff-eb8a6a0b9d6d83979e35e6b02a6eafbfL2000

andresilva

LGTM. Here's some comments for a future PR.

andresilva · 2018-08-14T15:26:46Z

uint/benches/bigint.rs

+#[macro_use]
+extern crate uint;
+construct_uint!(U128, 2);
+construct_uint!(U256, 4);


Can we use the exported types here? Apparently, creating the types here influences the benchmark results.

andresilva · 2018-08-14T15:27:11Z

uint/benches/bigint.rs

+#[macro_use]
+extern crate crunchy;
+#[macro_use]
+extern crate uint;


Add newline before macro invocation?

impl AsRef<Self> for primitives

dvdplm added 2 commits August 10, 2018 14:09

Remove asm and port over performance improvements from bigint

9609d2e

Add benchmark

16012b3

dvdplm self-assigned this Aug 10, 2018

dvdplm added F3-annoyance A0-pleasereview M4-core labels Aug 10, 2018

dvdplm requested review from andresilva, eira-fransham, NikVolf and debris August 10, 2018 12:21

dvdplm added 7 commits August 10, 2018 17:53

Port over no_std formatting and use rustc_hex 2.0 to make tests pass …

ae247ae

…in no_std

Run uint no_std tests

13d7356

Use rustc-hex without default features

3356726

move display/debug funtionality back to std-gated macro

4336907

don't run tests with --no-default-features

2af9e11

Try travis_wait to fix timeouts

1262de4

Debug travis

78cf6e7

eira-fransham previously requested changes Aug 13, 2018

View reviewed changes

dvdplm added 3 commits August 13, 2018 12:14

debug travis (attempt 3)

b49e0ed

Try with absurdly long wait time: 400min (attempt 4)

85ba498

Export U256 and U512

97ecf17

This was referenced Aug 13, 2018

Use uint instead of bigint lexfrl/parity-hash#9

Merged

Use uint openethereum/pwasm-ethereum#13

Merged

Use uint instead bigint openethereum/wasm-tests#52

Merged

dvdplm added 2 commits August 13, 2018 15:54

Add impl to converto from U256 to [u8; 32]

d3c335e

Put original travis config back

1906259

NikVolf reviewed Aug 13, 2018

View reviewed changes

NikVolf added A4-gotissues A4-clasignoffneeded and removed A0-pleasereview labels Aug 13, 2018

Fix repo meta-data key

b4b5c6f

Use --release to run tests

Move conversion inside macro

9261b85

debris approved these changes Aug 14, 2018

View reviewed changes

dvdplm added A0-pleasereview and removed A4-clasignoffneeded A4-gotissues labels Aug 14, 2018

eira-fransham approved these changes Aug 14, 2018

View reviewed changes

dvdplm merged commit 22209e1 into master Aug 14, 2018

andresilva reviewed Aug 14, 2018

View reviewed changes

debris deleted the chore/pick-changes-from-bigint branch August 14, 2018 15:39

dvdplm mentioned this pull request Aug 14, 2018

Use exported types in benchmark #26

Merged

sorpaas pushed a commit that referenced this pull request Jan 24, 2019

Merge pull request #25 from paritytech/impl_as_ref

f66e0ce

impl AsRef<Self> for primitives

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port over relevant changes from bigint #25

Port over relevant changes from bigint #25

dvdplm commented Aug 10, 2018 •

edited

Loading

eira-fransham left a comment

eira-fransham Aug 13, 2018

dvdplm Aug 13, 2018 •

edited

Loading

eira-fransham Aug 13, 2018

dvdplm Aug 13, 2018

dvdplm Aug 13, 2018

eira-fransham Aug 13, 2018

NikVolf commented Aug 13, 2018

dvdplm commented Aug 13, 2018

NikVolf Aug 13, 2018

NikVolf commented Aug 13, 2018 •

edited

Loading

Pzixel commented Aug 13, 2018 •

edited

Loading

dvdplm commented Aug 13, 2018

Pzixel commented Aug 13, 2018

eira-fransham commented Aug 14, 2018 •

edited

Loading

eira-fransham commented Aug 14, 2018 •

edited

Loading

Pzixel commented Aug 14, 2018 •

edited

Loading

eira-fransham commented Aug 14, 2018

Pzixel commented Aug 14, 2018

debris left a comment

dvdplm commented Aug 14, 2018

Pzixel commented Aug 14, 2018

andresilva left a comment

andresilva Aug 14, 2018

andresilva Aug 14, 2018

		@@ -4,27 +4,23 @@ homepage = "http://parity.io"
		repository = "https://github.com/paritytech/primitives"

Port over relevant changes from bigint #25

Port over relevant changes from bigint #25

Conversation

dvdplm commented Aug 10, 2018 • edited Loading

eira-fransham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvdplm Aug 13, 2018 • edited Loading

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

NikVolf commented Aug 13, 2018

dvdplm commented Aug 13, 2018

Choose a reason for hiding this comment

NikVolf commented Aug 13, 2018 • edited Loading

Pzixel commented Aug 13, 2018 • edited Loading

dvdplm commented Aug 13, 2018

Pzixel commented Aug 13, 2018

eira-fransham commented Aug 14, 2018 • edited Loading

eira-fransham commented Aug 14, 2018 • edited Loading

Pzixel commented Aug 14, 2018 • edited Loading

eira-fransham commented Aug 14, 2018

Pzixel commented Aug 14, 2018

debris left a comment

Choose a reason for hiding this comment

dvdplm commented Aug 14, 2018

Pzixel commented Aug 14, 2018

andresilva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvdplm commented Aug 10, 2018 •

edited

Loading

dvdplm Aug 13, 2018 •

edited

Loading

NikVolf commented Aug 13, 2018 •

edited

Loading

Pzixel commented Aug 13, 2018 •

edited

Loading

eira-fransham commented Aug 14, 2018 •

edited

Loading

eira-fransham commented Aug 14, 2018 •

edited

Loading

Pzixel commented Aug 14, 2018 •

edited

Loading