-
Notifications
You must be signed in to change notification settings - Fork 69
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
Deprecate heapsize #14
Conversation
everything is safer and probably needed).
@dvdplm I went into a full crate upgrade (memorydb being use by hashdb and hashdb by triedb we end up in a situation where it is needed to get everything same version (and iirc it was a consensus we keep all those crate versioned identically). |
@bkchr can you take a look at this please? :) |
pub static EMPTY_PREFIX: Prefix<'static> = &[]; | ||
|
||
/// Prefix byte information for avoding rc. | ||
pub type Prefix<'a> = &'a[u8]; |
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 you help me understand what this does?
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 is an additional information that can be use to ensure key uniqueness in the hashdb.
Basically we do not reference count the key value; if some different nodes in the trie contains the same content a pruning conflict will occur.
For ethereum, the constructed keys are long and unique enough so that leaf contains a part of the key big enough to ensure that it is hash is unique and no rc is needed (intermediatory node containing those leaf hash that is also the case). This really depends on the way keys are build.
In substrate where the use cases are more open, there was this pruning issue, solution there was to add the path in the trie to the info (unique by nature of the trie), we did not have to use reference count.
There trie issue with the merged pr contains more info (I did not understand directly so I asked my questions there).
So for parity ethereum, this is strictly overhead, note that with my trie pr about removal of extension, this overhead gets a bit reduced (there is also an interesting scheme that add the prefix in an ordered encoding to try to allow iteration directly from rocksdb but I do not know if it is worth the overhead (unless the state is totally pruned that does not allow state iteration still can be use for some analysis)).
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.
LGTM
This PR deprecates heapsize (currently keeping it behind an optional feature).
It uses parity-util-mem instead.
This PR can be reviewed but cannot merge until PRparitytech/parity-common#93paritytech/parity-common#94 get approved and the crate published.The versioning was not yet updated.