Merge bulk lookup proofs #163
Conversation
facebook-github-bot
added
the
CLA Signed
| let mut lookup_proofs = Vec::new(); | ||
| for i in 0..unames.len() { | ||
| lookup_proofs.push( | ||
| self.lookup_with_info::<H>( |
There was a problem hiding this comment.
Why do we need the info? I think if we've done the proper BFS loading, we should only need to batch-get the value states in order to load them into the cache, then we can do the regular proof generation operations since we'll only be accessing from local memory
There was a problem hiding this comment.
This is to eliminate duplicate get_user_state queries. One in identifying which labels are needed and one in (previously) lookup for the same purpose. This way pre-loaded info is passed to the lookup function, so no re-loads.
Looking at the code for MySQL's get_user_state, are we missing looking up the queried state in cache?
There was a problem hiding this comment.
Ah I see, yes we've excluded get_user_state from the caching logic because of the filter parameters. How would you know a cached value matches "<= epoch" or some weird filter without going to the backing store? It sounds like we might need a new query at the storage layer to retrieve the max versions for a collection of users which is <= a given epoch. (If I'm remembering right). We could move forward with this however and simply open an issue for it, but we'll want to do it properly for batch proof generation. Here's we will hit order N queries just to gather the user states, while not awful, still not ideal when we can do a better query.
There was a problem hiding this comment.
Sounds great, let's merge this in!
This PR adds support for bulk lookup proofs (See Issue #103).
First, for each lookup proof requested, necessary labels are calculated. Then, the nodes corresponding to their self- and sibling-prefixes are preloaded before the lookup operations (See the comments above the function
build_lookup_prefixes_setfor my understanding why this is the case).To test it, I added a new feature to the MySQL implementation where each storage call is tracked with its self-(
read/write) and data-type (e.g.,HistoryTreeNode). Here is its output with the following parameters:num_users: 50 num_epochs: 5 num_lookups: 100 running 1 test Metrics after publish(es). MySQL writes: 3279, MySQL reads: 6, Time read: 0.071272952 s, Time write: 1.13520928 s Tree size: 899 Node state count: 1386 Value state count: 250 Read call stats: [("get_direct:~Azks", 1), ("get_user_state_versions~", 5)] Write call stats: [("internal_batch_set~", 20), ("internal_set~", 3)] - Individual 100 lookups took 3103ms. Metrics after individual lookups: MySQL writes: 0, MySQL reads: 969, Time read: 2.041950956 s, Time write: 0 s Tree size: 899 Node state count: 1386 Value state count: 250 - Read call stats: [("get_direct:~Azks", 1), ("get_direct:~HistoryNodeState", 459), ("get_direct:~HistoryTreeNode", 459), ("get_user_state~", 50)] Write call stats: [] + Bulk 100 lookups took 1732ms. Metrics after lookup proofs: MySQL writes: 0, MySQL reads: 75, Time read: 0.59272097 s, Time write: 0 s Tree size: 899 Node state count: 1386 Value state count: 250 + Read call stats: [("batch_get~HistoryNodeState", 12), ("batch_get~HistoryTreeNode", 12), ("get_direct:~Azks", 1), ("get_user_state~", 50)] Write call stats: []Note the reduction in completion time (about 40% but may vary depending on the parameters) and
get_directcalls replaced withbatch_gets in bulk lookups.This code was a bit tricky to test since MySQL metrics are reset when printed and nodes in cache may change the resulting numbers. In addition, the users looked up might result in different time measurements. I tried and accounted for all that but if you notice any issues, please let me know!