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

Optimize performance #44

Open
Stentonian opened this issue Aug 24, 2023 · 7 comments
Open

Optimize performance #44

Stentonian opened this issue Aug 24, 2023 · 7 comments
Labels
Effort: much Large piece of work enhancement Making the current code better (e.g. performance)

Comments

@Stentonian
Copy link

Stentonian commented Aug 24, 2023

The performance of the Bulletproofs generation and verification is not something we have full control over in this repo so that is not the topic of this issue.

What we do have full control over is tree construction and path generation / verification. There is likely a bunch of optimization that can be done here if by just using better Rust progamming technique.

Currently the store of the tree is a hashmap but maybe this is not the best data structure to use. Reading material:

There may also be some ways to improve performance by minimizing the amount of data that is copied (rather passing pointers around). Reading material:

Another thing we can do to improve the tree constructor performance is try to find a pre-defined binary tree data structure that allows sparse trees and a custom merge function. This has been tried before (#12) and was not fruitful but it might be worth taking another look. It might be useful to read the BTree implementation code to get some inspiration on optimizations.

The thing with the tree is that we need to access it from the bottom up (for inclusion proof calculation), which seems to rule out linked nodes because of Rust's ownership model. But Rc<T> allows for multiple references to the same data. Rc doesn't work when doing concurrency but there seems to be an alternate to use there.

A lot of the structs defined in the code have an underlying 256 piece of data represented as a u8 array. Arrays of this type may be stored on the stack so we need to check that these are actually stored on the heap to prevent stack overflow. We could possibly wrap the values in a Box to make sure they are stored on the heap. Generally the data stored in these u8 arrays does not change, it only needs to change ownership. So the heap seems to be the best place for this data to be stored.

@Stentonian
Copy link
Author

Depending on how large the serialized tree is we may want to store only part of the tree. All we need to store really are the bottom-layer leaf nodes, but this means we have to reconstruct the tree each time we want to generate an inclusion proof. We could store the top n layers as well as the bottom layer, which would save on some of the construction time.

@Stentonian
Copy link
Author

Stentonian commented Sep 26, 2023

Interesting to note that de-referencing a heap variable well copy it to the stack (at least for a box): https://doc.rust-lang.org/rust-by-example/std/box.html

If you do the same memory check with a vector you see that it takes 24 bytes on the stack no matter how many elements you push to it:

    use std::mem;
    let mut a = Vec::<Coordinate>::new();
    for i in 0..10000 {
        a.push(Coordinate {
            x: i,
            y: i as u8,
        });
    }
    println!("Vec occupies {} bytes on the stack",
             mem::size_of_val(&a));

@Stentonian
Copy link
Author

A note from inside single_threaded::build_node

                // TODO may be able to further optimize by leaving out the padding leaf nodes
                // from the store.
                // Only insert nodes in the store if
                // a) node is a bottom layer leaf node (including padding nodes)
                // b) node is in one of the top X layers where X = store_depth
                // NOTE this includes the root node.

@Stentonian
Copy link
Author

A note from multi_threaded::get_num_nodes_left_of

// TODO can be optimized using a binary search

@Stentonian
Copy link
Author

Serialization/deserialization of the tree might be too slow at the input sizes we will be operating the code on, so it might be a good idea to try using a database as storage of the tree nodes as apposed to a hashmap. Some further details on other options here: #68

Also:

@Stentonian Stentonian added enhancement Making the current code better (e.g. performance) Effort: much Large piece of work labels Dec 1, 2023
@Stentonian
Copy link
Author

#137

@Stentonian
Copy link
Author

Stentonian commented Jan 19, 2024

Ideally we should move the storage of nodes to a database. It will allow instant save & load of the tree, at the cost of being more expensive to build.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Effort: much Large piece of work enhancement Making the current code better (e.g. performance)
Projects
None yet
Development

No branches or pull requests

1 participant