Use BucketQueueView in the protocol.
#2807
Conversation
| // The `TimestampedBundleInInbox` type contains 4 cryptohashes, 1 blockheight | ||
| // an index, two enums and the ChannelName. Only the ChannelName has an unbounded | ||
| // size but we can expect the size to be reasonably small, so a total size of 100 | ||
| // seems reasonable for the storing of the data. |
There was a problem hiding this comment.
We don't need this comment. It's going to be obsolete very quickly.
There was a problem hiding this comment.
I would probable even remove the first line.
| // an index, two enums and the ChannelName. Only the ChannelName has an unbounded | ||
| // size but we can expect the size to be reasonably small, so a total size of 100 | ||
| // seems reasonable for the storing of the data. | ||
| const TIMESTAMPBUNDLE_BUCKET_SIZE: usize = 100; |
There was a problem hiding this comment.
TIMESTAMPED_BUNDLES_BUCKET_SIZE
There was a problem hiding this comment.
We could also decide not create a constant altogether.
@afck What do you think?
There was a problem hiding this comment.
You mean just inline the 100? Or make it configurable? I think this is an example where I wouldn't make it configurable, since this is very hard to explain to the user and there is probably simply a range of reasonable values one of which we can fix.
There was a problem hiding this comment.
Yes, I was wondering if inlining the value wouldn't be just fine. Changing the value breaks the storage format -- which is internal to a validator but still...
| // The `BlockHeight` has just 8 bytes so the size is constant. | ||
| // This means that by choosing a size of 1000, we have a | ||
| // reasonable size that will not create any memory issues. | ||
| const BLOCKHEIGHT_BUCKET_SIZE: usize = 1000; |
| @@ -130,6 +130,7 @@ fn stored_indices<T>(stored_data: &VecDeque<(usize, Bucket<T>)>, position: usize | |||
| /// The size `N` has to be chosen by taking into account the size of the type `T` | |||
| /// and the basic size of a block. For example a total size of 100bytes to 10KB | |||
| /// seems adequate. | |||
| #[derive(Debug)] | |||
There was a problem hiding this comment.
Do we generally derive Debug for views?
MathieuDutSik
force-pushed
the
bucket_queue_view_protocol
branch
from
acc2648 to
d5da4ef
Compare
ma2bd
changed the title
BucketQueueView in the protocol.BucketQueueView in the protocol.
Motivation
The
BucketQueueViewallows to use queues more efficiently.Proposal
It is known that using Queues on Key-Value stores is kind of an antipattern for KeyValue stores.
Grouping entries into buckets can reduce this problem.
The problem is that grouping increases the memory cost and exposes to possible Out Of Memory. Thus we did the following:
QueueViewwithBlockHeightbeing the type, we use a bucket size of 1000 since BlockHeight is just 8 bytes.QueueViewwithTimestampedBundleInInboxwe use a bucket size of 100 since this type is essentially 4 Cryptohashes, a few number and a channel name which can be long but not too long.QueueViewwithMessageBundleno change is made as this type can be very large.It is to be pointed out that
BucketQueueViewhas two differences withQueueView:frontis no longer an async operation, which means the front is accessible just after theload. On the other hand thedelete_frontbecomes an async operation.Test Plan
(A) The CI is the recommended way.
(B) More tests would be needed for correctness.
(C) Runtime would be helpful to gauge the gain effectively obtained.
Release Plan
This PR changes the way that data is stored in the storage, therefore using it on existing TestNet / DevNet is impossible.
Links
None.