-
Notifications
You must be signed in to change notification settings - Fork 127
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
Utils buffer sv2 docs #1232
base: main
Are you sure you want to change the base?
Utils buffer sv2 docs #1232
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1232 +/- ##
==========================================
+ Coverage 19.30% 25.13% +5.83%
==========================================
Files 164 20 -144
Lines 10849 1134 -9715
==========================================
- Hits 2094 285 -1809
+ Misses 8755 849 -7906 Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Bencher Report
Click to view all benchmark results
|
Bencher Report
Click to view all benchmark results
|
Bencher Report
Click to view all benchmark results
|
bf95e9b
to
d8c1798
Compare
74fc730
to
3137545
Compare
db7a330
to
49ac424
Compare
Bencher Report
Click to view all benchmark results
|
Bencher Report
Click to view all benchmark results
|
Bencher Report
Click to view all benchmark results
|
Bencher Report
🚨 1 ALERT: Threshold Boundary Limit exceeded!
Click to view all benchmark results
|
5a30170
to
1ff713b
Compare
dfdb488
to
c7cc945
Compare
c7cc945
to
f8bc52e
Compare
f8bc52e
to
59c3fa3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partial review, I will complete it in some hours, but some answers to my comments would make me easier to complete the review 🙏
## Intro | ||
`BufferPool` is useful whenever we need to work with buffers sequentially (fill a buffer, get | ||
the filled buffer, fill a new buffer, get the filled buffer, and so on). | ||
`buffer_sv2` handles memory management for Stratum v2 (Sv2) roles. It provides a memory-efficient |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`buffer_sv2` handles memory management for Stratum v2 (Sv2) roles. It provides a memory-efficient | |
`buffer_sv2` handles memory management for Stratum V2 (Sv2) roles. It provides a memory-efficient |
4. Using the header and message to construct a | ||
[`framing_sv2::framing::Frame`](https://docs.rs/framing_sv2/2.0.0/framing_sv2/framing/enum.Frame.html). | ||
|
||
To fill the buffer, the `codec_sv2` decoder must bass a reference of the buffer to a filler. To |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To fill the buffer, the `codec_sv2` decoder must bass a reference of the buffer to a filler. To | |
To fill the buffer, the `codec_sv2` decoder must pass a reference of the buffer to a filler. To |
If so use it, then check if the second slot has been dropped, and so on. | ||
`BufferPool` is also optimized to drop all the slices. | ||
`BufferPool` is also optimized to drop the last slice. | ||
`BufferPool` can only be fragmented between the front and back and between back and end. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this mean? I'm having some problems in understanding how this back, front and end works tbh.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, only prefix and suffix slots can be used; intermediary slots are not supported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you expand more?
There are 8 slot | ||
#### Fragmentation, Overflow, and Optimization | ||
`BufferPool` can allocate a maximum of `8` `Slice`s (as it uses an `AtomicU8` to track used and | ||
freed slots) and up to the defined capacity in bytes. If all `8` slows are taken or there is no more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
freed slots) and up to the defined capacity in bytes. If all `8` slows are taken or there is no more | |
freed slots) and up to the defined capacity in bytes. If all `8` slots are taken or there is no more |
|
||
`BufferPool` is benchmarked against `BufferFromSystemMemory` and two additional structure for | ||
reference: `PPool` (a hashmap-based pool) and `MaxEfficeincy` (a highly optimized but unrealistic | ||
control implementation writen such that the benchmarks do not panic and the compiler does not |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
control implementation writen such that the benchmarks do not panic and the compiler does not | |
control implementation written such that the benchmarks do not panic and the compiler does not |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In CASE 1, we have:
12345678 BACK MODE
12345698 BACK MODE
Why don't we have:
12345678 BACK MODE
12345679 BACK MODE
?
|
||
// Retrieve the data as an owned slice | ||
let data_slice = buffer_pool.get_data_owned(); | ||
assert_eq!(buffer_pool.len(), 0); // Buffer is now empty |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it is not empty after you retrieve data with get_data_owned
?
let mut slices = VecDeque::new(); | ||
|
||
// Write data to fill back slots | ||
for _ in 0..4 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be for _ in 0..8
? Otherwise only 4 bytes are going to be written, but you defined a 8 bytes capacity. Maybe I'm missing something
// buffer | ||
writable.copy_from_slice(data_bytes); | ||
let data_slice = buffer_pool.get_data_owned(); // Take ownership of allocated segment | ||
slices.push_back(data_slice); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe you're adding the last 4 bytes here?
// Underlying buffer storing the data. | ||
inner: Vec<u8>, | ||
|
||
// Current cursor indicating where the next byte should be written. | ||
cursor: usize, | ||
|
||
// Starting index for the buffer. Useful for scenarios where part of the buffer is skipped or | ||
// invalid. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't these comments be public docs?
Addresses #1184.