-
Notifications
You must be signed in to change notification settings - Fork 3
Batch work #153
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
base: develop
Are you sure you want to change the base?
Batch work #153
Conversation
| // After this call, all_results will be left containing the elements [0, split_point) | ||
| // that's why we need to reverse the batches | ||
| let batch_results = all_results.split_off(split_point); | ||
| sender.send(Ok(batch_results)).map_err(|_| { |
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.
Used std::mem::take to empty the vector and move ownership of its content at the same time
this comment is only for clarification. Close it if fine
| // After this call, all_results will be left containing the elements [0, split_point) | ||
| // that's why we need to reverse the batches | ||
| let batch_results = all_results.split_off(split_point); | ||
| sender.send(Ok(batch_results)).map_err(|_| { |
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.
pub fn send(self, t: T) -> Result<(), T> {
self.inner.send(t)
}the send function has an indeed curious way of using the Err variant
this comment is only for clarification. Close it if fine
tbrezot
left a comment
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.
Okay, i did not go through with the reviewing: this MR is not in an acceptable state. Please, review your work and especially your diffs before submitting again, with a particular focus on:
- comments: do not repeat the code but if necessary explain the rationals behind it, spell check your comments, cleanup dead comments etc.
- naming: think through your naming convention to make it uniform and coherent. We talked about some names previously, please adopt them.
- do not modify files you do not need to modify! In particular be careful of the import statements.
I am asking a more-than-average code quality for the core Findex repository since it goes along with a published paper and its quality has an impact on our image. In the present state, this MR looks like a draft and is therefore not admissible for merging.
crate/findex/README.md
Outdated
|
|
||
| This crate provides the core functionality of Findex, defining the abstract data types, cryptographic operations, and encoding algorithms. | ||
|
|
||
| Findex also supports batch operations, allowing to index and search multiple items in a single request. This feature improves performance and efficiency when network time is a bottleneck. |
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.
This is not the reason as async should solve that. The reason is that too many file descriptor cannot be open in many linux distributions by default. Also, there might be an over due to instantiating a connection per request or limiting the number of truly parallel requests perform due to the use of a fixed-sized pool.
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.
New comment :
Findex also supports batching operations into a singe call to the memory interface, which reduces connection overhead and avoids file descriptor limits on some Linux systems.
crate/findex/src/batcher_findex.rs
Outdated
| // The underlying tests assume the existence of a `Findex` implementation that is correct | ||
| // The testing strategy for each function is to use the `Findex` implementation to perform the same operations | ||
| // and compare the results with the `BatcherFindex` implementation. | ||
| #[cfg(test)] | ||
| mod tests { |
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.
Write a single memory-generic test instead that performs something non-trivial.
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.
Write a single memory-generic test instead that performs something non-trivial.
Those are very basic unit tests. If one function has issues (say, delete) we want a test that instantly says "delete is wrong/bugged". A general integration test should rather (as you said) perform non trivial things and notify us upon failing that we have a logic issue
This "non trivial test" is also from the things I am planning next, I will make a list in my PR description
crate/findex/src/benches.rs
Outdated
| use crate::{Findex, IndexADT, MemoryEncryptionLayer, WORD_LENGTH, dummy_decode, dummy_encode}; | ||
| use crate::{ | ||
| Findex, IndexADT, WORD_LENGTH, dummy_decode, dummy_encode, | ||
| encryption_layer::MemoryEncryptionLayer, | ||
| }; |
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.
why this change?
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.
cargo fmt --all formats this, I don't really have an opinion on the matter
HatemMn
left a comment
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 reply, marked the comments that need thinking with a 🚀 and treated the (mostly) trivial matters + added a "roadmap" to the pr description
crate/findex/src/batcher_findex.rs
Outdated
| // Execute all futures concurrently and collect results | ||
| futures::future::try_join_all(search_futures).await?; |
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.
TBH I made this version without regards this matter and plan to think about the runtime problems once we finish discussion the algorithms, as it will introduce even more complexities and retain us from examining the more technically challenging parts - I should have mentionned this in my pr message sorry !
But no worries, I intentionally made it this way and am aware of what you said
crate/findex/src/batcher_findex.rs
Outdated
| // The underlying tests assume the existence of a `Findex` implementation that is correct | ||
| // The testing strategy for each function is to use the `Findex` implementation to perform the same operations | ||
| // and compare the results with the `BatcherFindex` implementation. | ||
| #[cfg(test)] | ||
| mod tests { |
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.
Write a single memory-generic test instead that performs something non-trivial.
Those are very basic unit tests. If one function has issues (say, delete) we want a test that instantly says "delete is wrong/bugged". A general integration test should rather (as you said) perform non trivial things and notify us upon failing that we have a logic issue
This "non trivial test" is also from the things I am planning next, I will make a list in my PR description
crate/findex/src/benches.rs
Outdated
| use crate::{Findex, IndexADT, MemoryEncryptionLayer, WORD_LENGTH, dummy_decode, dummy_encode}; | ||
| use crate::{ | ||
| Findex, IndexADT, WORD_LENGTH, dummy_decode, dummy_encode, | ||
| encryption_layer::MemoryEncryptionLayer, | ||
| }; |
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.
cargo fmt --all formats this, I don't really have an opinion on the matter
3ea52a5 to
92c3903
Compare
style: clippy and clleaning feat: improvements feat: rewrite lost files... feat: many work fix: final fixes fix: final fixes2 fix: fmt fix: delete useless sync traits fix: review fixes fix: first batch of fixes feat: second batch of fixes fix: adt file feat: BYE batcherarc
abd2acc to
60e640d
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.
Pull Request Overview
This PR introduces a batching layer implementation for memory operations, allowing multiple read/write operations to be grouped together for improved performance. The implementation separates different operation types and provides a clear distinction between input/output/operation types to maintain code readability.
Key changes:
- Implements a new batching layer with operation queueing and batch execution
- Adds
BatchingMemoryADTtrait for memory implementations that support batch operations - Creates
FindexBatcherfor batched index operations with concurrent execution using futures
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| crate/memories/src/lib.rs | Adds batching feature flag and BatchingMemoryADT trait definition |
| crate/memories/src/in_memory.rs | Implements BatchingMemoryADT for InMemory storage |
| crate/memories/src/batching_layer/operation.rs | Defines strongly-typed operation structures and enums |
| crate/memories/src/batching_layer/mod.rs | Module organization for batching layer components |
| crate/memories/src/batching_layer/memory.rs | Core MemoryBatcher implementation with operation management |
| crate/memories/src/batching_layer/error.rs | Error handling types for batching operations |
| crate/memories/src/batching_layer/buffer.rs | Thread-safe buffer implementation for operation queueing |
| crate/memories/Cargo.toml | Adds batch feature and futures dependency |
| crate/findex/src/lib.rs | Exports batching functionality when feature is enabled |
| crate/findex/src/findex.rs | Removes clippy allow directive |
| crate/findex/src/error.rs | Adds BatchFindexError for batching-specific errors |
| crate/findex/src/batcher_findex.rs | Implements FindexBatcher with batch operations and comprehensive tests |
| crate/findex/src/adt.rs | Defines IndexBatcher trait for batch operations |
| crate/findex/README.md | Documents batching functionality benefits |
| crate/findex/Cargo.toml | Adds batch feature and updates version dependencies |
| Cargo.toml | Updates workspace version and adds futures dependency |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
crate/findex/src/batcher_findex.rs
Outdated
| *self.encode, | ||
| *self.decode, |
Copilot
AI
Aug 25, 2025
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.
Attempting to dereference self.encode which is an Arc<Encoder>. This should be (*self.encode).clone() or the Encoder type needs to implement Copy.
| *self.encode, | |
| *self.decode, | |
| (*self.encode).clone(), | |
| (*self.decode).clone(), |
crate/findex/src/batcher_findex.rs
Outdated
| *self.encode, | ||
| *self.decode, |
Copilot
AI
Aug 25, 2025
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.
Attempting to dereference self.decode which is an Arc<Decoder>. This should be (*self.decode).clone() or the Decoder type needs to implement Copy.
| *self.encode, | |
| *self.decode, | |
| (*self.encode).clone(), | |
| (*self.decode).clone(), |
crate/findex/src/batcher_findex.rs
Outdated
| *self.encode, | ||
| *self.decode, |
Copilot
AI
Aug 25, 2025
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.
Same issue as above - attempting to dereference self.encode Arc without proper handling.
| *self.encode, | |
| *self.decode, | |
| self.encode.clone(), | |
| self.decode.clone(), |
crate/findex/src/batcher_findex.rs
Outdated
| *self.encode, | ||
| *self.decode, |
Copilot
AI
Aug 25, 2025
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.
Same issue as above - attempting to dereference self.decode Arc without proper handling.
| *self.encode, | |
| *self.decode, | |
| self.encode.clone(), | |
| self.decode.clone(), |
feat: i think it's finally working feat: errors fix: refactor tests and some more fixes fix: a lot of formatting fix: more formats and comments fix: more formats and comments2
0e99b6d to
6900efc
Compare
| // In simple terms, we *need* two separate implementations of batch_read$ | ||
| // This is way simpler to see for the write operation, as the guarded_write actual backend call will never | ||
| // happen in the first place. The manage function only calls batch_guarded_write. | ||
| pub inner: M, // The actual memory that does the R/W operations. |
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.
info for reviewers: there is no escape from the existence of the inner memory within this memory batcher.
Proof: look at the manage function, this line in particular:
rust let mut aggregated_reads_results = self.inner.batch_read(all_addresses).await?;
This performs the actual call to the memory (example, a Redis call).
This is different from the implementation of batch_read that is written below, which is just a fictive read
operation that upper Findex(or whatever program) that uses this will be calling.
In simple terms, we need two separate implementations of batch_read$
This is way simpler to see for the write operation, as the guarded_write actual backend call will never
happen in the first place. The manage function only calls batch_guarded_write.
Cargo.toml
Outdated
| criterion = { version = "0.6" } | ||
| smol-macros = { version = "0.1" } | ||
| tokio = { version = "1.46" } | ||
| futures = "0.3.31" # Todo: this is a temporary approach |
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.
this will be taken away on the next PR (or at least i hope so)
| @@ -1,5 +1,3 @@ | |||
| #![allow(clippy::type_complexity)] | |||
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.
an outdated lint, for some reason
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
tbrezot
left a comment
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.
There still may have some issues with the core logic. See my comments about the push function.
Otherwise, this is mostly style and readability improvements.
Cargo.toml
Outdated
|
|
||
| [workspace.package] | ||
| version = "8.0.1" | ||
| version = "8.0.2" |
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.
This should be a minor version, not a debug one.
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.
fixed
crate/findex/Cargo.toml
Outdated
| aes = "0.8" | ||
| cosmian_crypto_core.workspace = true | ||
| cosmian_sse_memories = { path = "../memories", version = "8.0" } | ||
| cosmian_sse_memories = { path = "../memories", version = "8.0.2" } # Should be above 8.0.2 to have batching features. |
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.
Using a minor will improve this.
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.
idk why but I don't have this comment on my files
I deleted it anyway
crate/findex/README.md
Outdated
|
|
||
| This crate provides the core functionality of Findex, defining the abstract data types, cryptographic operations, and encoding algorithms. | ||
|
|
||
| Findex also supports batching operations into a singe call to the memory interface, which reduces connection overhead and avoids file descriptor limits on some Linux systems. |
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.
Batching shouldn't be Findex-specific.
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.
I tried to change the sentence but can't find a better equivalent, do you suggest one ? Or I should delete this line ?
In my personnal opinion nothing in this sentence insinuates a tied-relashionship between findex and the batching layer
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.
i deleted the word findex
crate/findex/src/adt.rs
Outdated
| /// This trait extends the functionality of the standard `IndexADT` by | ||
| /// providing methods that operate on multiple keywords or entries | ||
| /// simultaneously. | ||
| #[cfg(feature = "batch")] | ||
| pub trait IndexBatcher<Keyword, Value> { |
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.
You say this is an extension, but this trait is not a super-trait of IndexADT. Maybe use another wording.
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.
new comment :
/// This trait provides methods that let an index operate on multiple keywords or entries simultaneously.
| /// Search the index for the values bound to the given keywords. | ||
| fn batch_search( | ||
| &self, | ||
| keywords: Vec<&Keyword>, |
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.
Best not to use a vector to avoid imposing needless collection upon the client.
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.
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.
Thanks to github bugs, A comment I wrote here was lost
I remember saying that we had 3 solutions :
- keep it as is
- accept to add lifetimes to the definition
- take ownership of the Keywords (I do not prefer doing this as it will induce non necessary cloning )
close this if you think we should keep it like it now
By the way,I agree with your comment and that I have implemented it for the other functions (batch_insert, etc...)
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.
Just a general note
I found myself also obliged to add "ExactSizeIterator" bound - basically garantees that we can call len() . This is mandatory, to know how big the memory should be instantiated
fn batch_insert<Values, Entries>(
&self,
entries: Entries,
) -> impl Send + Future<Output = Result<(), Self::Error>>
where
Values: Sync + Send + IntoIterator<Item = Value>,
Entries: Send + IntoIterator<Item = (Keyword, Values)>,
Entries::IntoIter: ExactSizeIterator;| let split_point = aggregated_reads_results.len() - input_addresses.len(); // This is the point where the last batch's results start. | ||
| let batch_results = aggregated_reads_results.split_off(split_point); // After this call, all_results will be left containing the elements [0, split_point). |
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.
Shorter names allow for clearer code:
| let split_point = aggregated_reads_results.len() - input_addresses.len(); // This is the point where the last batch's results start. | |
| let batch_results = aggregated_reads_results.split_off(split_point); // After this call, all_results will be left containing the elements [0, split_point). | |
| let batch_words = words.split_off(words.len() - batch_addresses.len()); |
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.
Okay for words but I honestly had trouble understanding what batch_addresses mean so I kept "input"
I also had to delete the comment above as you suggested
| // Upon failure, the vector we tried to send is returned in the Err variant, | ||
| // but it's explicitly ignored here to not extract information. | ||
| MemoryBatcherError::<M>::Channel( | ||
| "The receiver end of this read operation was dropped before the \ | ||
| `send` function could be called." | ||
| .to_owned(), | ||
| ) |
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.
| // Upon failure, the vector we tried to send is returned in the Err variant, | |
| // but it's explicitly ignored here to not extract information. | |
| MemoryBatcherError::<M>::Channel( | |
| "The receiver end of this read operation was dropped before the \ | |
| `send` function could be called." | |
| .to_owned(), | |
| ) | |
| MemoryBatcherError::<M>::ClosedChannel |
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.
done
| pub(crate) type WriteOperation<M> = ( | ||
| GuardedWriteInput<M>, | ||
| oneshot::Sender<Result<GuardedWriteOutput<M>, <M as MemoryADT>::Error>>, | ||
| ); |
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.
Is it a write or a guarded write?
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.
It's a "write", and to be exact it's a "Bulk of guarded writes" bundled together, which can also be called "batch_guarded_write" as the trait name
This looked too verbose tho, so I just said "write"
This isn't a public type anyway
crate/memories/src/lib.rs
Outdated
| #[allow(clippy::type_complexity)] // Refactoring this type will make the code unnecessarily more difficult to read without any actual benefit. | ||
| fn batch_guarded_write( | ||
| &self, | ||
| write_operations: Vec<( | ||
| (Self::Address, Option<Self::Word>), | ||
| Vec<(Self::Address, Self::Word)>, | ||
| )>, | ||
| ) -> impl Send + std::future::Future<Output = Result<Vec<Option<Self::Word>>, Self::Error>>; |
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.
You define the appropriate type in operation.rs:
| #[allow(clippy::type_complexity)] // Refactoring this type will make the code unnecessarily more difficult to read without any actual benefit. | |
| fn batch_guarded_write( | |
| &self, | |
| write_operations: Vec<( | |
| (Self::Address, Option<Self::Word>), | |
| Vec<(Self::Address, Self::Word)>, | |
| )>, | |
| ) -> impl Send + std::future::Future<Output = Result<Vec<Option<Self::Word>>, Self::Error>>; | |
| fn batch_guarded_write( | |
| &self, | |
| write_operations: Vec<GuardedWriteInput<Self>>, | |
| ) -> impl Send + std::future::Future<Output = Result<Vec<GuardedWriteOutput<Self>>, Self::Error>>; |
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.
That type was not public - I will make it public then
crate/memories/src/in_memory.rs
Outdated
| let mut res = Vec::with_capacity(operations.len()); | ||
| for (guard, bindings) in operations { | ||
| let (a, old) = guard; | ||
| let cur = store.get(&a).cloned(); | ||
| if old == cur { | ||
| for (k, v) in bindings { | ||
| store.insert(k, v); | ||
| } | ||
| } | ||
| res.push(cur); | ||
| } | ||
| Ok(res) |
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.
Use a map instead.
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.
done
HatemMn
left a comment
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.
Some changes were made according to the comments. Some of the "huge" changes were said in replies but not implemented till getting approval for them
This set of commits covers strictly the comments of the previous review, further commits might follow for the rest of the tasks (notably, getting rid of the futures crate that was there mostly for POC purposes)
Also please check out my alternative for the unreachable! statement and tell me if I shall revert the previous version or not, as the new one seems ( IMO ) worse of what was there
Cargo.toml
Outdated
|
|
||
| [workspace.package] | ||
| version = "8.0.1" | ||
| version = "8.0.2" |
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.
fixed
crate/findex/Cargo.toml
Outdated
| aes = "0.8" | ||
| cosmian_crypto_core.workspace = true | ||
| cosmian_sse_memories = { path = "../memories", version = "8.0" } | ||
| cosmian_sse_memories = { path = "../memories", version = "8.0.2" } # Should be above 8.0.2 to have batching features. |
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.
idk why but I don't have this comment on my files
I deleted it anyway
crate/findex/README.md
Outdated
|
|
||
| This crate provides the core functionality of Findex, defining the abstract data types, cryptographic operations, and encoding algorithms. | ||
|
|
||
| Findex also supports batching operations into a singe call to the memory interface, which reduces connection overhead and avoids file descriptor limits on some Linux systems. |
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.
I tried to change the sentence but can't find a better equivalent, do you suggest one ? Or I should delete this line ?
In my personnal opinion nothing in this sentence insinuates a tied-relashionship between findex and the batching layer
crate/findex/src/adt.rs
Outdated
| /// This trait extends the functionality of the standard `IndexADT` by | ||
| /// providing methods that operate on multiple keywords or entries | ||
| /// simultaneously. | ||
| #[cfg(feature = "batch")] | ||
| pub trait IndexBatcher<Keyword, Value> { |
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.
new comment :
/// This trait provides methods that let an index operate on multiple keywords or entries simultaneously.
| /// Search the index for the values bound to the given keywords. | ||
| fn batch_search( | ||
| &self, | ||
| keywords: Vec<&Keyword>, |
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.
crate/memories/src/in_memory.rs
Outdated
| let mut res = Vec::with_capacity(operations.len()); | ||
| for (guard, bindings) in operations { | ||
| let (a, old) = guard; | ||
| let cur = store.get(&a).cloned(); | ||
| if old == cur { | ||
| for (k, v) in bindings { | ||
| store.insert(k, v); | ||
| } | ||
| } | ||
| res.push(cur); | ||
| } | ||
| Ok(res) |
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.
done
| // Forward the BR/GW calls on Arcs to their actual implementations. | ||
| impl<M: BatchingMemoryADT + Sync + Send + Debug> MemoryADT for Arc<MemoryBatcher<M>> | ||
| where | ||
| M::Address: Send + Clone, | ||
| M::Word: Send + std::fmt::Debug, | ||
| { | ||
| type Address = M::Address; | ||
| type Error = MemoryBatcherError<M>; | ||
| type Word = M::Word; | ||
|
|
||
| async fn batch_read( | ||
| &self, | ||
| addresses: Vec<Self::Address>, | ||
| ) -> Result<Vec<Option<Self::Word>>, Self::Error> { | ||
| (**self).batch_read(addresses).await | ||
| } | ||
|
|
||
| async fn guarded_write( | ||
| &self, | ||
| guard: (Self::Address, Option<Self::Word>), | ||
| bindings: Vec<(Self::Address, Self::Word)>, | ||
| ) -> Result<Option<Self::Word>, Self::Error> { | ||
| (**self).guarded_write(guard, bindings).await | ||
| } | ||
| } |
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.
I deleted this and made it shareable by implementing the "clone" myself and putting the two inner variables within Arc<...>
| }; | ||
|
|
||
| struct Buffer<M: BatchingMemoryADT> { | ||
| capacity: usize, // the size at which the buffer should be flushed |
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.
done
crate/findex/README.md
Outdated
|
|
||
| This crate provides the core functionality of Findex, defining the abstract data types, cryptographic operations, and encoding algorithms. | ||
|
|
||
| Findex also supports batching operations into a singe call to the memory interface, which reduces connection overhead and avoids file descriptor limits on some Linux systems. |
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.
i deleted the word findex
| pub fn new(inner: M, n: usize) -> Self { | ||
| if n == 0 { | ||
| panic!("Buffer capacity must be greater than zero."); | ||
| }; |
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.
done

Updated summary (22/08/2025)
This new implementation of batcher_findex keeps the core idea of the previous one, but separates the different "inside" operations following the ideas stated in the PDF below :
memory_batcher.notes (2).pdf
However, due to the Rust compiler strict constraint on enums and the need to have a readable code that distinguishes an input/an output/an operation, some modifications have been done. So please take note of the comments and highlights that were added on top of the original file.
Other changes :
Next steps :