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

make long-running CPU tasks run on tokio's blocking threadpool. #133

Merged
merged 4 commits into from
Apr 24, 2024

Conversation

dan-da
Copy link
Collaborator

@dan-da dan-da commented Apr 7, 2024

Partially addresses #122.

This PR makes the following potentially CPU intensive operations execute on tokio's blocking threadpool, so they do not block the tokio executor:

  • create_transaction_from_data() which calls the prover
  • the inner mining loop (still single-threaded)
  • calls to triton_vm::program::Program::run() in Transaction

I am making this a PR rather than just pushing the commits for two reasons:

  1. bring greater team awareness to the need to run any lengthy CPU ops on tokio's threadpool (or separate OS thread) and examples/methodology for doing so.
  2. solicit input/suggestions as to other potentially lengthy/blocking computations. (please comment in Make long running CPU operations async friendly #122)
  3. spawn_blocking() expects a non-async fn, so I changed the (inner) mining loop so it doesn't make any async calls, which means it can't check the global state syncing flag. Instead the main loop notifies the (outer) mining loop when syncing starts and stops, which in turn halts or starts the (inner) mining loop which is running in tokio's threadpool. This should speed the inner mining loop a little, as it no longer needs to acquire a lock after computing each hash. Anyway, I thought @Sword-Smith may wish to review this change.

note: #130 has macros for logging fn call duration. Once that is merged, we can easily log duration of any suspected targets to determine which are worth moving to the blocking threadpool.

Commits:

    perf: add spawn_blocking around script execution
    
    Wraps spawn_blocking around triton::program::Program::run() calls
    within Transaction::validate_primitive_witness().
    
    This enables tokio to put potentially lengthy blocking code on its own
    thread.
    
    There are additional ::run() calls in consensus/mod.rs however
    they are within sync code, and spawn_blocking() can only be called
    from within an async fn. Making those async is a big refactor, so
    it is possible future work.
    perf: wrap mining loop with spawn_blocking()
    
    The mining loop is very CPU intensive and thus very async-unfriendly.
    
    We wrap the loop with spawn-blocking so it can execute on tokio's
    blocking threadpool and thus avoid blocking async tasks on the
    same thread.
    fix: send syncing start/stop messages to miner
    
    The mining loop is no longer async as it is wrapped by
    spawn_blocking().

    Thus it can no longer await async futures.  So we cannot check
    the GlobalState syncing flag, which requires an async lock
    acquisition.
    
    Instead, the main task sends StartSyncing and StopSyncing
    messages to the mining task which cancels the mining thread
    when it receives StartSyncing.
    perf: wrap tx prover with spawn_blocking
    
    The prover may take a very long time (minutes) to run and should not
    block the tokio executor.
    
    As such, we wrap `create_transaction_from_data()` with
    `spawn_blocking()` in order to execute this task in tokio's blocking
    threadpool.

Wraps spawn_blocking around triton::program::Program::run() calls
within Transaction::validate_primitive_witness().

This enables tokio to put potentially lengthy blocking code on its own
thread.

There are additional ::run() calls in consensus/mod.rs however
they are within sync code, and spawn_blocking() can only be called
from within an async fn. Making those async is a big refactor, so
it is possible future work.
The mining loop is very CPU intensive and thus very async-unfriendly.

We wrap the loop with spawn-blocking so it can execute on tokio's
blocking threadpool and thus avoid blocking async tasks on the
same thread.
The mining loop is no longer async as it is wrapped by
spawn_blocking().

Thus it can no longer await async futures.  So we cannot check
the GlobalState syncing flag, which requires an async lock
acquisition.

Instead, the main task sends StartSyncing and StopSyncing
messages to the mining task which cancels the mining thread
when it receives StartSyncing.
The prover may take a very long time (minutes) to run and should not
block the tokio executor.

As such, we wrap `create_transaction_from_data()` with
`spawn_blocking()` in order to execute this task in tokio's blocking
threadpool.
@Sword-Smith
Copy link
Member

Did you see my comment in the associated issue? I would like to make sure we agree on the fundamentals before proceeding with the specifics.

@Sword-Smith
Copy link
Member

Glad we agree on the fundamentals. On that note, LGTM.

@dan-da dan-da merged commit b160e1d into Neptune-Crypto:master Apr 24, 2024
3 checks passed
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.

None yet

2 participants