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

[noaov1]

This change is 

[codecov-commenter]

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 ⚠️

Additional details and impacted files

@@            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              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Yoni-Starkware]

Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @avi-starkware, @barak-b-starkware, and @noaov1)

---

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

        // Run.
        let results =
            Python::with_gil(|py| py.allow_threads(|| self.tx_executor().execute_txs(&txs)));

Something is weird here. You take the GIL just to suspend it.
According to the documentation, it Temporarily releases the GIL, thus allowing other Python threads to run.

So sounds like there should not be a problem while the GIL is not taken (which should be our case)

Non-blocking since it works

Code quote:

            Python::with_gil(|py| py.allow_threads(|| self.tx_executor().execute_txs(&txs)))

[Yoni-Starkware]

Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @avi-starkware, @barak-b-starkware, and @noaov1)

[avi-starkware]

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?

[Yoni-Starkware]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @avi-starkware, @barak-b-starkware, and @noaov1)

---

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

Previously, avi-starkware wrote…

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?

Don't we consume it before calling the executor?

[avi-starkware]

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]

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @barak-b-starkware and @noaov1)

[noaov1]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @avi-starkware, @barak-b-starkware, and @Yoni-Starkware)

---

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

Previously, avi-starkware wrote…

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.

You are right. I'm not sure why allow_threads is needed for running threads in rust. The docs suggest that rayon threads do not need to use allow_threads, maybe the std ones do?

[avi-starkware]

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 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]

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 :)