starkware-libs / blockifier

Blockifier is a Rust implementation for the transaction-executing component in the StarkNet sequencer, in charge of creating state diffs and blocks.
Apache License 2.0
171 stars 99 forks source link

fix(concurrency): allow threads in pyo3 (release the GIL) #1978

Closed noaov1 closed 1 month ago

noaov1 commented 1 month ago

This change is Reviewable

codecov-commenter commented 1 month ago

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 78.37%. Comparing base (4505d16) to head (076d771).

Files Patch % Lines
crates/native_blockifier/src/py_block_executor.rs 0.00% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1978 +/- ## ========================================== - Coverage 78.38% 78.37% -0.01% ========================================== Files 62 62 Lines 8855 8856 +1 Branches 8855 8856 +1 ========================================== Hits 6941 6941 - Misses 1475 1476 +1 Partials 439 439 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

avi-starkware commented 1 month ago

crates/native_blockifier/src/py_block_executor.rs line 232 at r1 (raw file): The problem is that the object txs_with_class_infos is of type &PyAny. See here:

  • Its lifetime represents the scope of holding the GIL which can be used to create Rust references that are bound to it, such as &PyAny.

Since the method execute_txs gets a reference to a Python object, we have to take the GIL before we can spawn threads.

However, as far as I understand, any access to a &PyAny requires taking the GIL again. @noaov1 are you sure this doesn't mean our threads get blocked by the GIL each time txs is read from?

avi-starkware commented 1 month ago

crates/native_blockifier/src/py_block_executor.rs line 232 at r1 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…
Don't we consume it before calling the executor?

You are right. txs is a purely rust variable. So we just need the with_gil to gain access to the Python token (py) from which we can run the method allow_threads.

Moreover, with_gil would not have been needed at all if the method execute_txs did not have pythonic objects as arguments.

avi-starkware commented 1 month ago

crates/native_blockifier/src/py_block_executor.rs line 232 at r1 (raw file):

Previously, noaov1 (Noa Oved) wrote…
You are right. I'm not sure why `allow_threads` is needed for running threads in rust. The [docs](https://pyo3.rs/v0.19.1/parallelism) suggest that rayon threads do not need to use `allow_threads`, maybe the std ones do?

The docs say that "pure rust functions" can use threads without worrying about the GIL, but functions that accept Python objects as arguments need to take the GIL before they can spawn a thread.

noaov1 commented 1 month ago

crates/native_blockifier/src/py_block_executor.rs line 232 at r1 (raw file):

Previously, avi-starkware wrote…
The docs say that "pure rust functions" can use threads without worrying about the GIL, but functions that accept Python objects as arguments need to take the GIL before they can spawn a thread.

I agree. I added the allow_threads code to a function that accepts Python objects but does not spawn any threads. It calls a rust method that spawns threads. BTW- it does not work using rayon threads as well (got timeout). Anyway it works :)