Allow shared Arc<Collector> usage#81
Open
yamabiiko wants to merge 1 commit intoibraheemdev:masterfrom
Open
Conversation
ibraheemdev
reviewed
Sep 17, 2025
| /// | ||
| /// Note that all `Guard` references used to access the map must be produced by | ||
| /// the provided `collector`. | ||
| pub fn collector<C: Borrow<Collector>>(self, collector: C) -> HashMapBuilder<K, V, S, C> { |
Owner
There was a problem hiding this comment.
This method is unsound without the extra bounds on K/V that were mentioned. It should also be distinct from the default collector builder, which takes an owned Collector without requiring extra bounds.
Author
There was a problem hiding this comment.
Sorry, I mistakenly pushed an outdated version. As I mentioned in the PR, setting an Arc<Controller> should be done via a distinct shared_controller
| // `HashMap` itself. If this was not true, we would require stricter bounds | ||
| // on `HashMap` operations themselves. | ||
| unsafe impl<K: Send + Sync, V: Send + Sync, S: Sync> Sync for HashMap<K, V, S> {} | ||
| unsafe impl<K, V, S: Sync, C: Sync + Borrow<Collector>> Sync for HashMap<K, V, S, C> {} |
| raw: raw::HashMap<K, V, S>, | ||
| pub struct HashMap<K, V, S = RandomState, C = Collector> | ||
| where | ||
| C: Borrow<Collector>, |
Owner
There was a problem hiding this comment.
I think I would prefer to just unconditionally store an Arc<Collector>. It's useful to limit the size of a HashMap on the stack anyways, and I don't think the extra generics are worth it.
Author
There was a problem hiding this comment.
Hmm, so you think it would be better to switch every HashMap to be initialized with an Arc<Collector>? I agree that the extra generics are not the cleanest.
Owner
There was a problem hiding this comment.
Sorry for the delay, yes, I think that would be reasonable. seize::Collector is also quite a large type, and so allocating it would be useful either way.
yamabiiko
force-pushed
the
master
branch
2 times, most recently
from
219c4b2 to
e7cdac0
Compare
yamabiiko
added a commit
to Ebi-Corp/ebi
that referenced
this pull request
Oct 3, 2025
- create new `FileSysService`, which handles retrieving or creating new nodes given a `ShelfDataRef` and a `PathBuf` - remove metadata from `File`, now load metadata on `FileSummary` - temporary use fork of `papapaya` for better memory usage ibraheemdev/papaya#81 - remove `dtag_files` from `Node` and `dtags` from file - change reference type of owned `Tag` to `SharedRef` to allow information edit - `attach`, `detach` for tags and dtags now take an input directory `FileId` instead of a path `PathBuf` - updated the respective calls in `RpcService` to query the `FileSysService` when required
yamabiiko
added a commit
to Ebi-Corp/ebi
that referenced
this pull request
Oct 13, 2025
- create new `FileSysService`, which handles retrieving or creating new nodes given a `ShelfDataRef` and a `PathBuf` - remove metadata from `File`, now load metadata on `FileSummary` - temporary use fork of `papapaya` for better memory usage ibraheemdev/papaya#81 - remove `dtag_files` from `Node` and `dtags` from file - change reference type of owned `Tag` to `SharedRef` to allow information edit - `attach`, `detach` for tags and dtags now take an input directory `FileId` instead of a path `PathBuf` - updated the respective calls in `RpcService` to query the `FileSysService` when required
yamabiiko
added a commit
to Ebi-Corp/ebi
that referenced
this pull request
Oct 13, 2025
- create new `FileSysService`, which handles retrieving or creating new nodes given a `ShelfDataRef` and a `PathBuf` - remove metadata from `File`, now load metadata on `FileSummary` - temporary use fork of `papapaya` for better memory usage ibraheemdev/papaya#81 - remove `dtag_files` from `Node` and `dtags` from file - change reference type of owned `Tag` to `SharedRef` to allow information edit - `attach`, `detach` for tags and dtags now take an input directory `FileId` instead of a path `PathBuf` - updated the respective calls in `RpcService` to query the `FileSysService` when required
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
As previously discussed in #66, this change is motivated by the memory usage of a freshly initialized
HashMaporHashSet, which is ~1kB. This is can cause memory usage to grow quite fast when defining multipleHashMapwithin a single struct.The change simply adds a
C: Borrow<Collector>generic to theHashMap, so in order to make the API safe we constrain such setting to ashared_controllerbuilder function with boundsK: Send + 'static, V: Send + 'static, which should be enough to enforce safety from my understanding.The implemented
Clonebehavior forHashMap<K, V, S, Arc<Collector>>is to simply create a newArc<Collector>rather than cloning it, as that seems the safer approach, even though it might be slightly confusing.