-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Implement Allocative
for the protocol views.
#4687
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
base: main
Are you sure you want to change the base?
Conversation
visitor.visit_field(Key::new("GenericCertificate_value"), &self.value); | ||
visitor.visit_field(Key::new("GenericCertificate_round"), &self.round); | ||
for (public_key, _) in &self.signatures { | ||
visitor.visit_field(Key::new("ValidatorPublicKey"), public_key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public keys also have a fixed size, don't they? Why assume the size for signatures but not public keys?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. Corrected.
af9011c
to
4772d6c
Compare
3155f45
to
c6e8523
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should be able to use derive
a bit more.
I'm not sure an Allocative
implementation should acquire locks. It's probably meant to be very fast. Anyway, if we do try_read
, we should never panic if we don't get the lock.
} | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would something like this work?
#[derive(Allocative)]
pub enum AccountPublicKey {
Ed25519(#[allocative(visit = visit_allocative_simple)] ed25519::Ed25519PublicKey),
}
fn visit_allocative_simple<'a, 'b, T>(_: &T, visitor: &'a mut Visitor<'b>) {
visitor.visit_simple_sized::<T>();
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, that works!
How do you find this?
3eeb6a9
to
f81a32f
Compare
f81a32f
to
2341ef4
Compare
/// The SECP256k1 public key sizes also used for validators. | ||
pub const SECP256K1_PUBLIC_KEY_SIZE: usize = std::mem::size_of::<secp256k1::Secp256k1PublicKey>(); | ||
/// The SECP256k1 signature sizes also used for validators. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// The SECP256k1 public key sizes also used for validators. | |
pub const SECP256K1_PUBLIC_KEY_SIZE: usize = std::mem::size_of::<secp256k1::Secp256k1PublicKey>(); | |
/// The SECP256k1 signature sizes also used for validators. | |
/// The secp256k1 public key sizes also used for validators. | |
pub const SECP256K1_PUBLIC_KEY_SIZE: usize = std::mem::size_of::<secp256k1::Secp256k1PublicKey>(); | |
/// The secp256k1 signature sizes also used for validators. |
(I think that's how we spell it elsewhere.)
/// The SECP256k1 public key sizes also used for validators. | ||
pub const SECP256K1_PUBLIC_KEY_SIZE: usize = std::mem::size_of::<secp256k1::Secp256k1PublicKey>(); | ||
/// The SECP256k1 signature sizes also used for validators. | ||
pub const SECP256K1_SIGNATURE_SIZE: usize = std::mem::size_of::<secp256k1::Secp256k1Signature>(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, do we still need those at all? Aren't those "simple", too?
(And even if: If it's only used in one place I'd rather use size_of
there directly?)
let signatures = self.signatures.deref(); | ||
for (public_key, _) in signatures { | ||
visitor.visit_field(Key::new("ValidatorPublicKey"), public_key); | ||
visitor.visit_simple(Key::new("ValidatorSignature"), 64); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use size_of
here instead of a number?
pub signatures: Cow<'a, [(ValidatorPublicKey, ValidatorSignature)]>, | ||
} | ||
|
||
impl Allocative for LiteCertificate<'_> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary because Allocative
doesn't support Cow
?
fn visit<'a, 'b: 'a>(&self, visitor: &'a mut Visitor<'b>) { | ||
visitor.visit_field(Key::new("LiteCertificate_value"), &self.value); | ||
visitor.visit_field(Key::new("LiteCertificate_round"), &self.round); | ||
let signatures = self.signatures.deref(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should only count it if its owned?
pub signature: ValidatorSignature, | ||
} | ||
|
||
impl<T: Allocative> Allocative for Vote<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we should be able to derive this, and either use visit_allocative_simple
or derive Allocative
for ValidatorSignature
, too?
_phantom: PhantomData<C>, | ||
#[allocative(visit = visit_allocative_simple)] | ||
stored_hash: Option<O>, | ||
#[allocative(visit = visit_allocative_simple)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(That will only count the stack size of the mutex, rather than its contents, will it? It would be more correct to add size_of::<Option<O>>()
to that. Since it's just a hash, maybe it doesn't matter much, but I'd at least add a short comment.
To be clear: In either case I agree that we do not need to acquire the mutex lock here!)
sizes: ByteMapView<C, u32>, | ||
#[allocative(visit = visit_allocative_simple)] | ||
stored_hash: Option<HasherOutput>, | ||
#[allocative(visit = visit_allocative_simple)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Same here.)
Motivation
Tracking the size of the objects being used is important for considering the memory usage and resolving the OOM.
Proposal
The
Allocative
trait is implemented for the views, and the code is followed from there.Several design decisions were made:
Test Plan
The CI.
More tests need to be added for tracking the memory usage in the operations.
Release Plan
The goal is to add it to the TestNet Conway.
Links
None.