Replace ElasticArray with SmallVec#46
Conversation
The elastic-array crate is a fine piece of software but it'd be good to prefer crates that are commonly used in the ecosystem and likely to be well maintained. smallvec is at v1.0 and generally well-regarded. Related PRs: - [parity-common #282](paritytech/parity-common#282) – [trie-db #46](paritytech/trie#46)
jimpo
left a comment
jimpo
left a comment
There was a problem hiding this comment.
I'm in favor of this switch. Though maybe it's also worth benchmarking this against regular Vec.
cheme
left a comment
cheme
left a comment
There was a problem hiding this comment.
The change looks good to me. On the other hand I got a small surprise when running a few of the bench, especially 'trie_mut_build_a', where perf decrease a bit. I observe that this bench is the one with bigger content stored, and testing with this branch (uses 'Vec' for 'DBValue'), I got my performance back and improvment on 'trie_mut_b'.
I think it is worth considering making DBValue Vec (in many case it ends up being converted to Vec).
I went ahead and implemented this in b18e2db and the terrible perf regression seems to have gone away. A bit unsure of how big the impact is on the substrate side though. |
4 participants
The
elastic-arraycrate is a fine piece of software but it'd be good to prefer crates that are commonly used in the ecosystem and likely to be well maintained.smallvecis atv1.0and generally well-regarded.Unfortunately as
DBValueis part of the public API oftrie-db, this is a breaking change so changing this implies annoying busy-work in consuming code.