Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Optimizing uint operations (architecture independent) #629

Merged
merged 14 commits into from Mar 15, 2016
Merged

Optimizing uint operations (architecture independent) #629

merged 14 commits into from Mar 15, 2016

Conversation

tomusdrw
Copy link
Collaborator

@tomusdrw tomusdrw commented Mar 7, 2016

Related to #490

Starting from:

test u128_mul      ... bench:   2,305,342 ns/iter (+/- 107,978)
test u256_add      ... bench:     135,920 ns/iter (+/- 9,476)
test u256_full_mul ... bench:  15,597,671 ns/iter (+/- 593,695)
test u256_mul      ... bench:   5,241,255 ns/iter (+/- 316,925)
test u256_sub      ... bench:     545,754 ns/iter (+/- 44,429)
test u512_add      ... bench:     191,064 ns/iter (+/- 10,624)
test u512_sub      ... bench:   1,627,907 ns/iter (+/- 82,830)

Current:

test u128_mul      ... bench:      45,772 ns/iter (+/- 1,508)
test u256_add      ... bench:      30,052 ns/iter (+/- 2,552)
test u256_full_mul ... bench:   3,005,477 ns/iter (+/- 98,643)
test u256_mul      ... bench:     101,664 ns/iter (+/- 7,941)
test u256_sub      ... bench:      32,904 ns/iter (+/- 1,382)
test u512_add      ... bench:      78,783 ns/iter (+/- 3,864)
test u512_sub      ... bench:      59,469 ns/iter (+/- 2,340)

vs ASM

test u128_mul      ... bench:      52,687 ns/iter (+/- 2,566)
test u256_add      ... bench:      38,811 ns/iter (+/- 2,439)
test u256_full_mul ... bench:   2,482,358 ns/iter (+/- 141,787)
test u256_mul      ... bench:     124,951 ns/iter (+/- 6,652)
test u256_sub      ... bench:      38,449 ns/iter (+/- 1,872)
test u512_add      ... bench:      96,885 ns/iter (+/- 6,237)
test u512_sub      ... bench:      96,400 ns/iter (+/- 5,347)

@tomusdrw tomusdrw added the A0-pleasereview 🤓 Pull request needs code review. label Mar 7, 2016
NikVolf
NikVolf reviewed Mar 8, 2016
View reviewed changes
util/src/lib.rs
@@ -167,3 +167,59 @@ pub use io::*;
pub use log::*;
pub use kvdb::*;

#[cfg(test)]
mod tests {
Copy link
Contributor

Choose a reason for hiding this comment

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

why move here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Those tests were not compiling in bigint crate - they are for H256 which is implemented in util/src/hash.rs

@NikVolf
Copy link
Contributor

NikVolf commented Mar 8, 2016 

test u512_add      ... bench:      79,473 ns/iter (+/- 2,289)
test u512_sub      ... bench:         461 ns/iter (+/- 14)

why so much difference?

@NikVolf NikVolf added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Mar 8, 2016
@debris
Copy link
Collaborator

debris commented Mar 8, 2016

Maybe benchmark code was incorrectly inlined?

@tomusdrw
Copy link
Collaborator Author

tomusdrw commented Mar 8, 2016

Yeah, it must have been some heavy optimization by compiler (the subtraction was done only for most significant u64). I've modified the bench and updated results.

@NikVolf NikVolf added A8-looksgood 🦄 Pull request is reviewed well. and removed A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Mar 9, 2016
@NikVolf
Copy link
Contributor

NikVolf commented Mar 9, 2016

looks good to me, though with next pr we should move fixed hash integers to bigint crate and return tests back there as well

@gavofyork
Copy link
Contributor

will hold off until after 1.0.

@gavofyork gavofyork added the A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. label Mar 9, 2016
@gavofyork
Copy link
Contributor

conflicts need sorting.

@gavofyork gavofyork added A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. and removed A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. A8-looksgood 🦄 Pull request is reviewed well. labels Mar 14, 2016
Merge branch 'master' into uint_opt
d5ec177 
Conflicts:
	test.sh
	util/bigint/src/uint.rs
@tomusdrw tomusdrw added A0-pleasereview 🤓 Pull request needs code review. and removed A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. labels Mar 14, 2016
@NikVolf NikVolf added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Mar 15, 2016
gavofyork added a commit that referenced this pull request Mar 15, 2016
@gavofyork
Merge pull request #629 from ethcore/uint_opt
e98cfd8 
Optimizing uint operations (architecture independent)
@gavofyork gavofyork merged commit e98cfd8 into master Mar 15, 2016
@gavofyork gavofyork deleted the uint_opt branch March 15, 2016 10:24
@tomusdrw tomusdrw mentioned this pull request Mar 25, 2016
Closed
@MagaTailor MagaTailor mentioned this pull request Mar 25, 2016
Closed
@MagaTailor
Copy link

I've updated my benchmark results in https://github.com/ethcore/parity/issues/596#issuecomment-192630065, well done!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@tomusdrw @NikVolf @debris @gavofyork @MagaTailor