Remove buf redux #2179

kruss · 2025-01-27T10:39:17Z

This PR removes the chipmunk dependencies to buf_redux.

AmmarAbouZor

Great work 👍
I've found some minor issues with naming, formatting and missing comments alongside with a suggestion to avoid undefined behavior inside one of the proptests

AmmarAbouZor · 2025-01-30T12:25:53Z

application/apps/indexer/addons/bufread/README.md

@@ -0,0 +1,2 @@
+# bufread
+A buffered reader implementation in Rust with special behavior.


Do we need to provide more infos in the readme?

AmmarAbouZor · 2025-01-30T12:38:01Z

application/apps/indexer/addons/bufread/benches/reader_benches.rs

+    let source_min_size = 100 * MAX_PACKET_LEN;
+    let buffer_max_size = 3 * MAX_PACKET_LEN;
+    let buffer_min_size = MAX_PACKET_LEN;


I'm not sure if having black_box() on the parser only is enough here. Can you try to have the values here black-boxed as well? I think we may get different result with them black-boxed

let source_min_size = black_box(100 * MAX_PACKET_LEN); ...

AmmarAbouZor · 2025-01-30T12:47:27Z

application/apps/indexer/addons/bufread/src/lib.rs

+        unsafe {
+            vec.set_len(cap);
+        }


I think it's a good practice to write a safety comment on each unsafe code block link

AmmarAbouZor · 2025-01-30T12:50:31Z

application/apps/indexer/addons/bufread/src/lib.rs

+        &self.slice[self.start..self.end]
+    }
+
+    /// Sets the amount of newly read bytes.


Explanation about the returned value is missing. Also variable name num in code below could be more expressive

AmmarAbouZor · 2025-01-30T13:00:55Z

application/apps/indexer/addons/bufread/src/lib.rs

+    pub fn write_from(&mut self, buffer: &[u8]) -> usize {
+        let size = min(self.write_available(), buffer.len());
+        self.write_slice()[..size].copy_from_slice(&buffer[..size]);
+        self.write_done(size)
+    }
+
+    /// Returns the number of currently available bytes for writing.
+    pub fn write_available(&self) -> usize {
+        self.slice.len() - self.end
+    }
+
+    /// Returns the current slice to write to.
+    pub fn write_slice(&mut self) -> &mut [u8] {
+        &mut self.slice[self.end..]
+    }
+
+    /// Sets the amount of newly written bytes.
+    pub fn write_done(&mut self, size: usize) -> usize {
+        let before = self.end;
+
+        self.end = min(self.end + size, self.slice.len());
+        self.end - before
+    }


Method names could be a little bit confusing because they do different kind of jobs but have same names:

write_available() and write_slice() are both getters

write_done() is a setter

write-from() actually writes the data into internal buffer.

I think having the function names starts with get, do, set or mark would be helpful to understand their purpose.
The same is for read function above as well

AmmarAbouZor · 2025-01-30T13:15:28Z

application/apps/indexer/addons/bufread/tests/proto.rs

+        assert!(payload_len <= MAX_PAYLOAD_LEN);
+
+        let packet_len = HEADER_LEN + payload_len;
+        let mut packet: Vec<u8> = Vec::with_capacity(packet_len);
+        unsafe {
+            packet.set_len(packet_len);
+        }
+
+        let header = (payload_len as u16).to_be_bytes().to_vec();
+        packet[0] = header[0];
+        packet[1] = header[1];
+
+        rand::thread_rng().fill_bytes(&mut packet[HEADER_LEN..]);


The bytes in the slice packet[2..HEADER_LEN] are uninitialized memory which may lead to undefined behavior. Is this intended? In that case we may need some comments in code here to explain the intention behind that.

Note: I would consider initialize the packet here with zeros instead the unsafe code since performance here isn't that relevant

AmmarAbouZor · 2025-01-30T13:21:38Z

application/apps/indexer/addons/text_grep/src/buffer.rs

-    policy::{DoRead, ReaderPolicy},
-    Buffer,
-};
+// TODO this duplicates: application/apps/indexer/processor/src/search/buffer.rs


Just to make sure this todo is intended to be done later and hadn't got forgotten 😄

AmmarAbouZor · 2025-01-30T13:29:51Z

application/apps/indexer/addons/text_grep/src/buffer.rs

+impl<R: Read> BufRead for CancellableBufReader<R> {
+    fn fill_buf(&mut self) -> Result<&[u8]> {
+        if self.cancel.is_cancelled() {
+            return Ok(&[][..]);


This can be

return Ok(&[]);

AmmarAbouZor · 2025-01-30T13:33:49Z

application/apps/indexer/addons/text_grep/src/buffer.rs

@@ -1,39 +1,62 @@
-use buf_redux::{


Formatting seems to be broken on some files. Can you please run cargo fmt in indexer directory.
Running that on my machine introduced changes to the files:

addons/text_grep/src/buffer.rs

processor/src/search/buffer.rs

processor/src/search/searchers/mod.rs

AmmarAbouZor · 2025-01-30T13:35:50Z

application/apps/indexer/processor/src/search/buffer.rs

+impl<R: Read> BufRead for CancellableBufReader<R> {
+    fn fill_buf(&mut self) -> Result<&[u8]> {
+        if self.cancel.is_cancelled() {
+            return Ok(&[][..]);


Like the other file the formatting is off and this line can be

return Ok(&[]);

AmmarAbouZor · 2025-01-30T13:47:14Z

@kruss @marcmo
It may be worth it to look into miri to see if we can use it since we are introducing unsafe code in this PR

Remove buf_redux

61adc97

kruss force-pushed the remove_buf_redux branch from 9282c27 to 9db429e Compare January 27, 2025 11:45

Add bufread as an addon

fdeca27

kruss force-pushed the remove_buf_redux branch from 9db429e to fdeca27 Compare January 29, 2025 08:16

kruss requested review from DmitryAstafyev and AmmarAbouZor January 29, 2025 08:24

AmmarAbouZor requested changes Jan 30, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove buf redux #2179

Remove buf redux #2179

kruss commented Jan 27, 2025

AmmarAbouZor left a comment

AmmarAbouZor Jan 30, 2025

AmmarAbouZor Jan 30, 2025

AmmarAbouZor Jan 30, 2025

AmmarAbouZor Jan 30, 2025

AmmarAbouZor Jan 30, 2025

AmmarAbouZor Jan 30, 2025

AmmarAbouZor Jan 30, 2025

AmmarAbouZor Jan 30, 2025

AmmarAbouZor Jan 30, 2025

AmmarAbouZor Jan 30, 2025

AmmarAbouZor commented Jan 30, 2025 •

edited

Loading

		@@ -0,0 +1,2 @@
		# bufread
		A buffered reader implementation in Rust with special behavior.

Remove buf redux #2179

Are you sure you want to change the base?

Remove buf redux #2179

Conversation

kruss commented Jan 27, 2025

AmmarAbouZor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmmarAbouZor commented Jan 30, 2025 • edited Loading

AmmarAbouZor commented Jan 30, 2025 •

edited

Loading