Skip to content
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

(#52) Optimize unspent utxo #150

Merged
merged 5 commits into from
Nov 18, 2019
Merged

(#52) Optimize unspent utxo #150

merged 5 commits into from
Nov 18, 2019

Conversation

eugene-babichenko
Copy link
Contributor

@eugene-babichenko eugene-babichenko commented Nov 12, 2019

  • Implement SparseArray.
  • Implement wrapper (FastSparse) on top of sparse array that allows to share data between its instances and thus allows for faster clones.

Data under FastSparseArray is shared until set or shrink function is called. It is also possible to avoid copying the underlying structure when calling set, but actual use cases do not require that.

TBD

Resolves #52

This implementation of sparse arrays is intended to store 256 elements
at most. It relies on 256-bit bitmap that stores indexes of elements
that actually exist and uses popcount to restore indexes in the
underlying vector.

SparseArray implements an iterator.
This wrapper allows to share underlying data storage until it is
required to shrink. Shrinks and additions to the storage trigger actual
clone operations.
Copy link
Contributor

@NicolasDP NicolasDP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

sparse-array/src/fast.rs Outdated Show resolved Hide resolved
sparse-array/src/fast.rs Outdated Show resolved Hide resolved
#[derive(Clone, Debug, PartialEq, Eq)]
pub struct SparseArray<V> {
index: BitmapIndex,
data: Vec<V>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be reduced to a Box<[V]> to save 8 bytes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about that. Will we still be able to add and remove elements at arbitrary positions?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, the Box<[V]> is for when the array is immutable/frozen. when in the tree, it's useful to reduce the cost of the array by 8 bytes by leaf. But once you want to update it, you can thaw it into a Vec with into_vec

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will change the interface a bit. Instead of set(&mut self, idx: u8, value: V) and remove(&mut self, idx: u8) -> Option<V> we will have set(self, idx: u8, value: V) -> Self and remove(self, idx: u8) -> (Self, Option<V>).

Anyway, I think that's ok for an immutable struct.

Copy link
Member

@vincenthz vincenthz Nov 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I think that's fine. if you want to have a mutable interface, I usually like the pattern of having a Builder type which use data structure that are more friendly to modification like Vec in this case.

@eugene-babichenko
Copy link
Contributor Author

@vincenthz I used where clauses for functions that really require the Clone trait, it allowed to removed the trait bound from the majority of structs. The only structure in utxo that still require Clone is Ledger, but each method here require Clone, so I think this is fine.

@eugene-babichenko
Copy link
Contributor Author

@vincenthz I suggest moving this

Special bitmaps for small arrays (8, 16, ... elements)

into a separate issue and then create a reusable bitmap that will be used by both sparse-array and imhamt.

@vincenthz
Copy link
Member

yes, we can move the special cases to another low priority issue.

Otherwise everything has been addressed and now ready for merge, but I'm less than conviced about the latest commit to move from u8 to usize. it's safer to keep as the lowest possible "size" since it's always safe to cast to bigger size, whereas the opposite direction usize -> u8 is not.

@eugene-babichenko
Copy link
Contributor Author

dropped this one

@vincenthz vincenthz merged commit 5939bbb into master Nov 18, 2019
@vincenthz
Copy link
Member

nice one !

@vincenthz vincenthz deleted the optimize-unspent-utxo branch December 8, 2019 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optimise Utxo representation
3 participants