-
Notifications
You must be signed in to change notification settings - Fork 165
pinocchio: Add pubkey_eq helper
#248
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,7 @@ | ||
| //! Public key type and functions. | ||
|
|
||
| use core::ptr::read_unaligned; | ||
|
|
||
| use crate::program_error::ProgramError; | ||
|
|
||
| /// Number of bytes in a pubkey. | ||
|
|
@@ -33,6 +35,24 @@ pub fn log(pubkey: &Pubkey) { | |
| core::hint::black_box(pubkey); | ||
| } | ||
|
|
||
| /// Compare two `Pubkey`s for equality. | ||
| /// | ||
| /// The implementation of this function is currently more efficient | ||
| /// than `p1 == p2` since it compares 8 bytes at a time instead of | ||
| /// byte-by-byte. | ||
|
Comment on lines
+40
to
+42
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this is just a more efficient implementation of // Comparing `Pubkey`s 8 bytes at a time is faster, than comparing byte at a
// time. One would expect LLVM to do this optimization for us, but it does not
// at the moment (as of platform tools v1.43). Comparing keys is a pretty
// common operation, so we want to make it faster if we can.
impl PartialEq for Pubkey {
fn eq(&self, other: &Self) -> bool {
let p1 = self.as_ptr() as *const u64;
let p2 = other.as_ptr() as *const u64;
// SAFETY: `p1` and `p2` both point to `[u8; 32]` bytes of data, which
// is the same size as `[u64; 4]`.
unsafe {
read_unaligned(p1) == read_unaligned(p2)
&& read_unaligned(p1.add(1)) == read_unaligned(p2.add(1))
&& read_unaligned(p1.add(2)) == read_unaligned(p2.add(2))
&& read_unaligned(p1.add(3)) == read_unaligned(p2.add(3))
}
}
}Bikeshedding Maybe it is possible to get the same effect if you just help the compiler a bit, // Comparing `Pubkey`s 8 bytes at a time is faster than comparing byte at a
// time. One would expect LLVM to do this optimization for us, but it does not
// at the moment (as of platform tools v1.43). Comparing keys is a pretty
// common operation, so we want to make it faster if we can.
impl PartialEq for Pubkey {
fn eq(&self, other: &Self) -> bool {
let p1 = self.as_ptr() as *const [u64; 4];
let p2 = other.as_ptr() as *const [u64; 4];
// SAFETY: `p1` and `p2` both point to `[u8; 32]` bytes of data, which
// is the same size as `[u64; 4]`.
unsafe { read_unaligned(p1) == read_unaligned(p2) }
}
}I would also add use static_assertions::assert_eq_size;
// Comparing `Pubkey`s 8 bytes at a time is faster than comparing byte at a
// time. One would expect LLVM to do this optimization for us, but it does not
// at the moment (as of platform tools v1.43). Comparing keys is a pretty
// common operation, so we want to make it faster if we can.
impl PartialEq for Pubkey {
fn eq(&self, other: &Self) -> bool {
let p1 = self.as_ptr() as *const [u64; 4];
let p2 = other.as_ptr() as *const [u64; 4];
assert_eq_size!(*self, [u64; 4]);
assert_eq_size!(*other, [u64; 4]);
// SAFETY: `self` and `[u64; 4]` are of the same size.
unsafe { read_unaligned(p1) == read_unaligned(p2) }
}
}(It is a compile time assertion with no runtime code.)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wanted this, too – at first. It would be great to get the savings everywhere devs use But since our Also, sounds like Lucas is looking into this, so maybe this discussion will be irrelevant soon.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We have done this for the
Unfortunately no, it is even worse. 😅 I think the compiler tries to do something different when it sees an array, which in the end becomes "bloated" and uses more CU. You can use this simple bench to see the effect. As @rustopian mentioned, Lucas will have a look at this to see if we can improve the code generation so the compiler does the "right" thing.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This is out of scope for this PR obviously, but, considering
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It will be very soon: #118 😊 |
||
| #[inline(always)] | ||
| pub const fn pubkey_eq(p1: &Pubkey, p2: &Pubkey) -> bool { | ||
| let p1_ptr = p1.as_ptr() as *const u64; | ||
| let p2_ptr = p2.as_ptr() as *const u64; | ||
|
|
||
| unsafe { | ||
| read_unaligned(p1_ptr) == read_unaligned(p2_ptr) | ||
| && read_unaligned(p1_ptr.add(1)) == read_unaligned(p2_ptr.add(1)) | ||
| && read_unaligned(p1_ptr.add(2)) == read_unaligned(p2_ptr.add(2)) | ||
| && read_unaligned(p1_ptr.add(3)) == read_unaligned(p2_ptr.add(3)) | ||
| } | ||
| } | ||
febo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| /// Find a valid [program derived address][pda] and its corresponding bump seed. | ||
| /// | ||
| /// [pda]: https://solana.com/docs/core/cpi#program-derived-addresses | ||
|
|
||
febo marked this conversation as resolved.
Show resolved
Hide resolved
|
Uh oh!
There was an error while loading. Please reload this page.