Skip to content
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

Allow multithreading #6

Closed
wants to merge 1 commit into from
Closed

Allow multithreading #6

wants to merge 1 commit into from

Conversation

davidbrochart
Copy link
Collaborator

@davidbrochart davidbrochart commented Sep 28, 2023

Allowing parallelism in Rust-only code fails:

🍹 Building a mixed python/rust project
🔗 Found pyo3 bindings
🐍 Found CPython 3.11 at /home/david/github/davidbrochart/pycrdt/.pixi/env/bin/python
📡 Using build options features from pyproject.toml
Ignoring pytest: markers 'extra == "test"' don't match your environment
Ignoring y-py: markers 'extra == "test"' don't match your environment
Ignoring mypy: markers 'extra == "test"' don't match your environment
   Compiling pycrdt v0.1.0 (/home/david/github/davidbrochart/pycrdt)
error[E0277]: `NonNull<Branch>` cannot be sent between threads safely
   --> src/text.rs:9:1
    |
9   | #[pyclass]
    | ^^^^^^^^^^ `NonNull<Branch>` cannot be sent between threads safely
    |
    = help: within `text::Text`, the trait `Send` is not implemented for `NonNull<Branch>`
note: required because it appears within the type `BranchPtr`
   --> /home/david/.cargo/registry/src/index.crates.io-6f17d22bba15001f/yrs-0.16.10/src/types/mod.rs:172:12
    |
172 | pub struct BranchPtr(NonNull<Branch>);
    |            ^^^^^^^^^
note: required because it appears within the type `TextRef`
   --> /home/david/.cargo/registry/src/index.crates.io-6f17d22bba15001f/yrs-0.16.10/src/types/text.rs:94:12
    |
94  | pub struct TextRef(BranchPtr);
    |            ^^^^^^^
note: required because it appears within the type `Text`
   --> src/text.rs:10:12
    |
10  | pub struct Text {
    |            ^^^^
note: required by a bound in `ThreadCheckerStub`
   --> /home/david/.cargo/registry/src/index.crates.io-6f17d22bba15001f/pyo3-0.19.2/src/impl_/pyclass.rs:894:33
    |
894 | pub struct ThreadCheckerStub<T: Send>(PhantomData<T>);
    |                                 ^^^^ required by this bound in `ThreadCheckerStub`
    = note: this error originates in the attribute macro `pyclass` (in Nightly builds, run with -Z macro-backtrace for more info)

For more information about this error, try `rustc --explain E0277`.
error: could not compile `pycrdt` (lib) due to previous error
💥 maturin failed
  Caused by: Failed to build a native library through cargo
  Caused by: Cargo build finished with "exit status: 101": `PYO3_ENVIRONMENT_SIGNATURE="cpython-3.11-64bit" PYO3_PYTHON="/home/david/github/davidbrochart/pycrdt/.pixi/env/bin/python" PYTHON_SYS_EXECUTABLE="/home/david/github/davidbrochart/pycrdt/.pixi/env/bin/python" "cargo" "rustc" "--features" "pyo3/extension-module" "--message-format" "json-render-diagnostics" "--manifest-path" "/home/david/github/davidbrochart/pycrdt/Cargo.toml" "--lib"`

I suspect that this is because Text.push is passed a reference to a TransactionMut, which cannot be sent between threads?
Any idea @Horusiath?

@davidbrochart davidbrochart marked this pull request as draft September 28, 2023 07:50
@Horusiath
Copy link

Horusiath commented Sep 28, 2023

Technically Send/Sync traits are auto implemented automatically by the type if all of its components also implement them. But when we boil them down into core properties, at some point you must manually guarantee that type is Send/Sync safe.

  1. One way is to essentially do unsafe impl Send for MyBranchPtr {} - most of the time this would work because BranchPtr never moves in memory (it's pinned). So as long as corresponding collection is not deleted or the Doc is not dropped, BranchPtr is safe.
  2. Another way of forcing Send/Sync safety in rust is to wrap type with Mutext<>. The downside is that you always need to acquire thread lock before accessing its contents. So you could wrap every collection like struct PyArray(Mutex<ArrayRef>) and pass it over to py.allow_threads.

@davidbrochart
Copy link
Collaborator Author

Thanks @Horusiath.
I think I'm going to leave that for a potential future improvement. pycrdt is a mixed Python/Rust project anyway, and I'm not even sure the performance gain of running Rust in parallel would be noticeable with the Python overhead.

@davidbrochart davidbrochart deleted the multithreading branch October 13, 2023 09:30
@davidbrochart davidbrochart restored the multithreading branch December 11, 2023 09:48
@davidbrochart
Copy link
Collaborator Author

Reopening as this might be fixed in Yrs (see y-crdt/ypy#142 (comment)).

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