Skip to content

Add optional sync feature to allow sharing DocumentsRef.#138

Draft
cgranade wants to merge 1 commit intoPaligo:mainfrom
cgranade:cgranade/sync-feature
Draft

Add optional sync feature to allow sharing DocumentsRef.#138
cgranade wants to merge 1 commit intoPaligo:mainfrom
cgranade:cgranade/sync-feature

Conversation

@cgranade
Copy link

@cgranade cgranade commented Jan 6, 2026

This draft PR adds a new Cargo feature that, when enabled, uses Arc and RwLock instead of Rc and RefCell to allow sharing DocumentsRef across threads. In particular, this allows DocumentsRef to be stored as a field in a struct marked with PyO3 as #[pyclass], as that attribute requires DocumentsRef: Send + Sync.

By making synchronization conditioned on a feature, existing single-thread performance can be preserved, while allowing consuming libraries that need synchronization primitives to explicitly opt-in to that functionality.

To enable the new feature, this PR also adds a new trait, BorrowWith, that differs from the std::borrow::Borrow trait by allowing implementers to use custom types as borrow targets as long as those custom types satisfy the appropriate lifetimes. This trait is incomplete, missing bounds to ensure that the target types are appropriate for use as borrowed references and comments indicating the purpose of the trait; it is included regardless so as to solicit initial discussions.

See also: #66

@faassen
Copy link
Collaborator

faassen commented Jan 7, 2026

Thank you! Some feedback:

  • I'm leaning towards making Arc the standard behavior rather than having a feature, taking the performance costs and making maintenance more easy. We don't have much in the way of benches yet anyway... The main impact will likely be the document order system. We need to do some analysis in where it's being called.

  • I admit I'm fuzzy about BorrowWith, and how it fits into this picture.

@cgranade
Copy link
Author

cgranade commented Jan 7, 2026

Thanks for your comments, I really appreciate it! As for making it a feature or not, I'm happy to refactor my PR without using a feature — that would require replacing Rc by Arc as well as RefCell by RwLock, since RefCell is inherently single-threaded. Using a lock is probably more impactful to performance than using Arc, but I don't have any data to back that hunch up. If it's helpful, I can look at adding a criterion.rs suite or similar to help obtain that data?

With respect to BorrowWith, that's only used to offer a uniform API when the sync feature is turned on or off, and can be eliminated if sync isn't exposed as a Cargo feature. std::borrow::Borrow is almost enough that this PR didn't need to add a new trait, but it makes the assumption that the target type for a borrow is always &T for some T, which isn't true when the target type is a lock guard. Since std::borrow::Borrow uses that assumption to make sure that the lifetime of the returned reference is at least as long as that of &self, I used a generic lifetime parameter on the BorrowWith trait to enforce that the target type lives at least as long as &self whether or not the sync feature is enabled.

Regardless, I'm happy to remove that trait, whatever is most helpful. ♥

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants