Add NodeHandle enum for node references within Node #35
Conversation
There was a problem hiding this comment.
Running same bench as in #34, the perf from #34 degrades (8%), I am not sure about the origin of it but it could have been related to the fact that children have to decode their inline nodes but the bench is on iterator so it should go into all inline node, so it is probably not that (unless there is multiple calls to that).
Regarding the PR, I agree that this try_decode_hash should not be a codec method. Maybe there is a way to put it in another trait like Layout or somewhere else, to avoid those static rules (size == lenght(hash)) in code.
| @@ -100,72 +100,84 @@ where | |||
| { | |||
| // load an inline node into memory or get the hash to do the lookup later. | |||
| fn inline_or_hash<C, H>( | |||
| node: &[u8], | |||
| parent_hash: H::Out, | |||
There was a problem hiding this comment.
'parent_hash' as a pointer here?
| NodeHandle::Hash(hash) | ||
| }, | ||
| EncodedNodeHandle::Inline(data) => { | ||
| let child = Node::from_encoded::<C, H>(parent_hash, data, db, storage)?; |
There was a problem hiding this comment.
is it possible that the small perf hit comes from getting a decoded data handle when previously it was encoded data? I don't really think so (exept if from branch this method is called on child we do not need, but it should not), but I am not sure where it comes from.
| child(4), child(5), child(6), child(7), | ||
| child(8), child(9), child(10), child(11), | ||
| child(12), child(13), child(14), child(15), | ||
| child(0)?, child(1)?, child(2)?, child(3)?, |
There was a problem hiding this comment.
I believe the performance hit may come for the fact that before this pr, call to child here only went into try_decode_hash that do not do inline node decoding.
After this pr for all inline node (even if not needed) we run a decoding step.
There was a problem hiding this comment.
But it shouldn't matter for iteration since all child are queried, for other operation it can, but it is probably minor.
|
No sorry, I misread the code, there is no such static rules. |
|
Needs a rebase |
jimpo
force-pushed
the
jimpo/decode-hash
branch
from
41a51f8 to
42574e6
Compare
|
Performance hit might come from |
Currently, branch and extension nodes just have a byte slice reference to the child nodes and one uses the
NodeCodec::try_decode_hashmethod to determine whether it is a hash or an inline node reference. Instead, we make this an explicit part of the decoded Node structure to simply code and make the codec more flexible. For example, we can stop encoding the length of the hashes inside branch nodes and instead use another bitfield inside branch nodes to indicate whether each child is a hash or inline reference, which should save space.This is another breaking change to NodeCodec building on #34.