Multi-limb arithmetic for runtime #3743

kianenigma · 2019-10-02T14:15:03Z

This adds a new minimal type, Number to the arithmetic primitive types of the runtime that can be used to store potentially infinitely large unsigned (or not #3716) integers and safely apply the basic operations on them. Number can be built from all numeric types and provide try_into all typical numeric types.

The main objective was to support an always accurate
pub fn multiply_by_rational(a: u128, b: u128, c: u128) -> Result<u128, &'static str> while this PR brings more. The main reason is that to have a multiply and division you need both the addition and subtraction for large numbers as well. And the abstraction added is the minimum needed to have a reasonably readable code. The only --potential-- downside is that maybe a super-ugly code that was specialised for only two limbs of u128 would have been slightly faster due to loop unrolling etc, but the algorithms would have been the same.

before it can properly be reviewed:

deal with the few remaining TODOs
actually use Number in multiply_by_rational.
decide if Number is even a good name 😕

The code is already big I so I will probably not do it here but if this gets merged, there should be follow-up PRs to:

relocate all the arithmetic stuff into a separate crate.
create better and more fuzzing code for all tests that bombard the code with random values.

core/sr-primitives/src/sr_arithmetic.rs

…b-arithmetic

kianenigma · 2019-10-07T14:40:12Z

Should be now ready for review.

cc @expenses

jimpo

Haven't reviewed division or tests yet. Generally very nicely commented IMO.

There seems to be lots of room for memory efficiency improvements. I think switching to little-endian order (least significant limbs at the front of the vec) would make a lot of stuff easier.

jimpo · 2019-10-07T15:05:22Z

core/sr-primitives/src/sr_arithmetic.rs

+		/// limbs. The caller may strip the output if desired.
+		///
+		/// Taken from "The Art of Computer Programming" by D.E. Knuth, vol 2, chapter 4.
+		pub fn add(mut self, mut other: Self) -> Self {


Why does this consume other instead of taking other: &Self?

I am generally more in favour of consuming when an arithmetic operation cannot fail. If it might fail, then it is more sensible to take a reference.

But now seeing the big picture here, I suppose I could also take all the rhss by reference.

currently the code does resize input and so modify them. maybe we can actually implement resize in another way taking only references but otherwise taking ownership seems to make sense to me here

You don't need to resize if get returns a default of 0 for out-of-range indices.

resize cannot change (it must change the caller somehow) and get should be a raw getter preferably, similar to other std types. We can have a checked_get() -> Option<_> and unwrap_or over it.

I am quite doubtful still regarding accepting arguments as ref or owning.
For the reason mentioned above, I prefer owning. But since this type can own a large vector of numbers moving stuff around is for sure non-sensical. Moreover, this type is still a number and the std implementation of those takes everything by value (thou' this is not a strong argument as those all have Copy).

Hence, if we are to value speed over consistency, I don't see it fair to only make other a ref and still consume self. Both will be ref. I am in favour of implementing this.

core/sr-primitives/src/sr_arithmetic.rs

kianenigma · 2019-10-07T15:50:59Z

There seems to be lots of room for memory efficiency improvements. I think switching to little-endian order (least significant limbs at the front of the vec) would make a lot of stuff easier.

Oh big time. but I intentionally did not (and rather not) include it in this initial PR in favour of brevity. But would like to hear more about your opinion on it.

EDIT: One of the resources that I had for optimisation but have iced for now, and is an interesting read https://skanthak.homepage.t-online.de/division.html

xlc · 2019-10-07T21:23:46Z

Will it better to move this into a standalone crate? So that other non-substrate projects (smart contracts) can use this as well

expenses · 2019-10-08T00:08:00Z

core/sr-primitives/src/sr_arithmetic.rs

+		}
+	}
+
+	impl ops::Add for Number {


Do these traits need to be implemented? I think ideally you want to encourage adds, muls etc. that use references instead of taking ownership. Otherwise looks good.

yeah I was being too biased here with my personal preference of taking ownership when an operation always returns. I will make all the rhs's reference.

I have to see, maybe we can also not do it here and leave it for some benchmarks to clearly show the difference and then refactor to reference in some future PR.

It'd be preferable to choose the API in this PR and leave refactors in future PRs to just optimize implementation without changing API ideally.

yes but anyway this is not a big deal. Number implements Clone, changing the API doesn't really break code, user just have to make some more clone and borrow in part of its code that's all.

kianenigma · 2019-10-08T11:36:16Z

Will it better to move this into a standalone crate? So that other non-substrate projects (smart contracts) can use this as well

@xlc see above:

The code is already big I so I will probably not do it here but if this gets merged, there should be follow-up PRs to:

relocate all the arithmetic stuff into a separate crate.
create better and more fuzzing code for all tests that bombard the code with random values.

gui1117

yes impressive clarity.
I haven't review mul and div yet though

core/sr-primitives/src/sr_arithmetic.rs

gui1117 · 2019-10-08T11:54:04Z

core/sr-primitives/src/sr_arithmetic.rs

+		/// limbs. The caller may strip the output if desired.
+		///
+		/// Taken from "The Art of Computer Programming" by D.E. Knuth, vol 2, chapter 4.
+		pub fn add(mut self, mut other: Self) -> Self {


currently the code does resize input and so modify them. maybe we can actually implement resize in another way taking only references but otherwise taking ownership seems to make sense to me here

core/sr-primitives/src/sr_arithmetic.rs

kianenigma · 2019-10-08T15:06:52Z

decide if Number is even a good name 😕

Anyone with an opinion on this^?

core/sr-primitives/src/sr_arithmetic.rs

gui1117 · 2019-10-08T15:48:26Z

decide if Number is even a good name confused

Anyone with an opinion on this^?

We could also use BigNum, or BigUnsigned, or BigUint as in num-bigint crate 🤔

Maybe better:
UsignedMultiLimb, UMultiLimb.

kianenigma · 2019-10-08T16:47:35Z

decide if Number is even a good name confused

Anyone with an opinion on this^?

We could also use BigNum, or BigUnsigned, or BigUint as in num-bigint crate 🤔

BigUint seems the most reasonable.

kianenigma · 2019-10-09T09:17:08Z

API has been changed to (self, other: &mut Self) in all cases for now. I might still try and remove the mutability requirement by eliminating resize, but other than that I won't touch anything else. Lots of improvements to made listed all here #3745

Also refactored to BigUint

gui1117 · 2019-10-09T09:36:32Z

taking mutable reference seems a bit weird to me, let's say you only have a immutable value you will have to clone it to add it to another one.

Maybe you could remove the resize by introducing a function iterating_resized which only takes reference to BigUInt and iterate on its digits as if it was resized.

kianenigma · 2019-10-09T13:10:38Z

taking mutable reference seems a bit weird to me, let's say you only have a immutable value you will have to clone it to add it to another one.

muts are gone. I see the current state of the PR as a good fit to get in.

…b-arithmetic

core/sr-primitives/src/sr_arithmetic.rs

gui1117 · 2019-10-09T14:22:28Z

core/sr-primitives/src/sr_arithmetic.rs

+		/// - requires `other` to be stripped and have no leading zeros.
+		/// - requires `self` to be stripped and have no leading zeros.
+		/// - requires `other` to have at least two limbs.
+		/// - requires `self` to have a greater or equal length compared to `other`.


I feel those constrains are quite strong maybe this could be reduced.

Actually I wonder if we want user to use this BigUInt directly maybe the most exposed API should be with less contrained as require and returning stripped output always ?

like we would rename this div_stripped_multiple_limbs and expose a div which only constrain is that other is different from 0 otherwise it returns 0

core/sr-primitives/src/sr_arithmetic.rs

Co-Authored-By: thiolliere <[email protected]>

core/sr-primitives/src/sr_arithmetic.rs

… into kiz-limb-arithmetic

core/sr-primitives/src/sr_arithmetic.rs

gui1117 · 2019-10-09T17:06:39Z

core/sr-primitives/src/sr_arithmetic.rs

+		/// A naive getter for limb at `index`. Note that the order is lsb -> msb.
+		///
+		/// #### Panics
+		///
+		/// This panics if index is out of range.
+		pub fn get(&self, index: usize) -> Single {
+			self.digits[self.len() - 1 - index]
+		}


maybe I could be in favor of silently returning zero instead of panic but I'm also fine with that.

Rational was to be act similar to a raw [] operator. I suppose with checked_get() it should be fine.

…b-arithmetic

kianenigma added 3 commits October 1, 2019 13:47

First working version of all operations.

e2ae7f4

New and improved version of everything.

eea5dff

Minor cleanup.

f7c34d5

kianenigma commented Oct 2, 2019

View reviewed changes

core/sr-primitives/src/sr_arithmetic.rs Outdated Show resolved Hide resolved

kianenigma commented Oct 2, 2019

View reviewed changes

core/sr-primitives/src/sr_arithmetic.rs Outdated Show resolved Hide resolved

Fix build

7b318b9

kianenigma mentioned this pull request Oct 3, 2019

Fuzzing and Organisation for Runtime Arithmetic Types #3745

Closed

3 tasks

kianenigma added 4 commits October 4, 2019 11:34

Finalize nignum

b27a819

Some final works on refactors and tests.

34bcce4

Merge branch 'master' of github.com:paritytech/substrate into kiz-lim…

d4d2cf1

…b-arithmetic

fix build

ae5cd09

kianenigma marked this pull request as ready for review October 7, 2019 14:39

kianenigma added the A0-please_review Pull request needs code review. label Oct 7, 2019

jimpo reviewed Oct 7, 2019

View reviewed changes

expenses suggested changes Oct 8, 2019

View reviewed changes

gui1117 reviewed Oct 8, 2019

View reviewed changes

kianenigma added 2 commits October 8, 2019 15:28

Some review comments

f9cf95c

Bench, better try into and nits

426e68f

gui1117 reviewed Oct 8, 2019

View reviewed changes

core/sr-primitives/src/sr_arithmetic.rs Show resolved Hide resolved

gui1117 reviewed Oct 8, 2019

View reviewed changes

core/sr-primitives/src/sr_arithmetic.rs Outdated Show resolved Hide resolved

mutify the API

8d0905a

rename to big_uint

72da381

kianenigma added 2 commits October 9, 2019 15:08

unmutify.

4e8e5cf

Remove resize

68db2fc

kianenigma requested a review from gui1117 October 9, 2019 13:10

Merge branch 'master' of github.com:paritytech/substrate into kiz-lim…

c0330ea

…b-arithmetic

gui1117 reviewed Oct 9, 2019

View reviewed changes

kianenigma commented Oct 9, 2019

View reviewed changes

core/sr-primitives/src/sr_arithmetic.rs Outdated Show resolved Hide resolved

kianenigma and others added 2 commits October 9, 2019 16:36

Apply suggestions from code review

15499b2

Update core/sr-primitives/src/sr_arithmetic.rs

affe102

Co-Authored-By: thiolliere <[email protected]>

gui1117 reviewed Oct 9, 2019

View reviewed changes

core/sr-primitives/src/sr_arithmetic.rs Outdated Show resolved Hide resolved

kianenigma added 2 commits October 9, 2019 18:23

BEtter proof

aecd627

Merge branch 'kiz-limb-arithmetic' of github.com:paritytech/substrate…

157f3fc

… into kiz-limb-arithmetic

gui1117 approved these changes Oct 9, 2019

View reviewed changes

gui1117 reviewed Oct 9, 2019

View reviewed changes

kianenigma added 3 commits October 10, 2019 09:32

Fix panic doc.

3822a50

Merge branch 'master' of github.com:paritytech/substrate into kiz-lim…

6245fd4

…b-arithmetic

Bump.

6009807

paritytech-ci added the B2-breaksapi label Oct 10, 2019

kianenigma merged commit 66042f1 into master Oct 10, 2019

kianenigma deleted the kiz-limb-arithmetic branch October 10, 2019 07:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multi-limb arithmetic for runtime #3743

Multi-limb arithmetic for runtime #3743

kianenigma commented Oct 2, 2019 •

edited

Loading

kianenigma commented Oct 7, 2019

jimpo left a comment

jimpo Oct 7, 2019

kianenigma Oct 8, 2019

gui1117 Oct 8, 2019

jimpo Oct 8, 2019

kianenigma Oct 8, 2019

kianenigma commented Oct 7, 2019 •

edited

Loading

xlc commented Oct 7, 2019

expenses Oct 8, 2019

kianenigma Oct 8, 2019

kianenigma Oct 8, 2019

jimpo Oct 8, 2019

gui1117 Oct 8, 2019

kianenigma commented Oct 8, 2019

gui1117 left a comment

gui1117 Oct 8, 2019

kianenigma commented Oct 8, 2019

gui1117 commented Oct 8, 2019 •

edited

Loading

kianenigma commented Oct 8, 2019

kianenigma commented Oct 9, 2019 •

edited

Loading

gui1117 commented Oct 9, 2019

kianenigma commented Oct 9, 2019

gui1117 Oct 9, 2019

gui1117 Oct 9, 2019

kianenigma Oct 10, 2019

Multi-limb arithmetic for runtime #3743

Multi-limb arithmetic for runtime #3743

Conversation

kianenigma commented Oct 2, 2019 • edited Loading

kianenigma commented Oct 7, 2019

jimpo left a 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

Choose a reason for hiding this comment

kianenigma commented Oct 7, 2019 • edited Loading

xlc commented Oct 7, 2019

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

kianenigma commented Oct 8, 2019

gui1117 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kianenigma commented Oct 8, 2019

gui1117 commented Oct 8, 2019 • edited Loading

kianenigma commented Oct 8, 2019

kianenigma commented Oct 9, 2019 • edited Loading

gui1117 commented Oct 9, 2019

kianenigma commented Oct 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kianenigma commented Oct 2, 2019 •

edited

Loading

kianenigma commented Oct 7, 2019 •

edited

Loading

gui1117 commented Oct 8, 2019 •

edited

Loading

kianenigma commented Oct 9, 2019 •

edited

Loading