Neptune-Crypto / neptune-core

anonymous peer-to-peer cash
Apache License 2.0
25 stars 7 forks source link

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

Closed dan-da closed 5 months ago

dan-da commented 6 months ago

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:

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 #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.
Sword-Smith commented 5 months ago

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 commented 5 months ago

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