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

Optimized hash lookups #4144

Merged
merged 2 commits into from
Jan 13, 2017
Merged

Optimized hash lookups #4144

merged 2 commits into from
Jan 13, 2017

Conversation

arkpar
Copy link
Collaborator

@arkpar arkpar commented Jan 12, 2017

Profiling shows that eq is not compiled into the most efficient code and is on a hot path.

@arkpar arkpar added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Jan 12, 2017
@NikVolf NikVolf added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 12, 2017
@@ -62,6 +62,8 @@ pub fn clean_0x(s: &str) -> &str {
}
}

extern { fn memcmp(s1: *const i8, s2: *const i8, n: usize) -> i32; }
Copy link
Contributor

Choose a reason for hiding this comment

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

should use the libc typedefs rather than Rust native types.

Copy link
Contributor

Choose a reason for hiding this comment

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

and actually, since the libc dependency is necessary to do this correctly, we should just use https://doc.rust-lang.org/libc/x86_64-unknown-linux-gnu/libc/fn.memcmp.html

rather than linking it in ad-hoc

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

memcmp is intrinsic and does not actually require libc. I'd prefer not to add a dependency that is not really needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

it is needed if we want this code to be future-proof/cross-platform

@rphmeier rphmeier added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Jan 12, 2017
@arkpar arkpar added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Jan 12, 2017
@rphmeier rphmeier added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 12, 2017
@gavofyork gavofyork merged commit cfb6dd2 into master Jan 13, 2017
@gavofyork gavofyork deleted the hash-opt branch January 13, 2017 08:52
arkpar added a commit that referenced this pull request Jan 16, 2017
* Optimize hash comparison

* Use libc
gavofyork pushed a commit that referenced this pull request Jan 16, 2017
* verification: check if server is running (#4140)

* verification: check if server is running

See also ethcore/email-verification#67c6466 and ethcore/sms-verification#a585e42.

* verification: show in the UI if server is running

* verification: code style ✨, more i18n

* fix i18n key

* Optimized hash lookups (#4144)

* Optimize hash comparison

* Use libc

* Ropsten fork detection (#4163)

* Stop flickering + added loader in AddressSelector (#4149)

* Stop UI flickering + added loader to AddressSelector #4103

* PR Grumbles

* Add a password strength component (#4153)

* Added new PasswordStrength Component

* Added tests

* PR Grumbles

* icarus -> update, increase web timeout. (#4165)

* icarus -> update, increase web timeout.

* Fix estimate gas

* Fix token images // Error in Contract Queries (#4169)

* Fix dapps not loading (#4170)

* Add secure to dappsreg

* Remove trailing slash // fix dapps
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants