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

try to do deserialization of transaction in a rayon thread #4801

Merged
merged 5 commits into from
Jul 21, 2022
Merged

Conversation

oxarbitrage
Copy link
Contributor

@oxarbitrage oxarbitrage commented Jul 20, 2022

Motivation

We want to add the deserialization of transactions and blocks in the zebra-network crate into the rayon pool. Resolves #4787

Solution

Implement deserialize transactions and blocks in a rayon thread.

Review

@oxarbitrage
Copy link
Contributor Author

This code fails compilation with the following:

oxarbitrage@oxarbitrage-Lenovo-ideapad-320S-14IKB:~/zebra/issue4787/zebra$ cargo test
   Compiling zebrad v1.0.0-beta.12 (/home/oxarbitrage/zebra/issue4787/zebra/zebrad)
   Compiling zebra-network v1.0.0-beta.12 (/home/oxarbitrage/zebra/issue4787/zebra/zebra-network)
error[E0597]: `body` does not live long enough
   --> zebra-network/src/protocol/external/codec.rs:413:51
    |
413 |                 let mut body_reader = Cursor::new(&body);
    |                                       ------------^^^^^-
    |                                       |           |
    |                                       |           borrowed value does not live long enough
    |                                       argument requires that `body` is borrowed for `'static`
...
465 |             }
    |             - `body` dropped here while still borrowed

error[E0597]: `body_reader` does not live long enough
   --> zebra-network/src/protocol/external/codec.rs:430:63
    |
430 |                     b"tx\0\0\0\0\0\0\0\0\0\0" => self.read_tx(&mut body_reader),
    |                                                  -------------^^^^^^^^^^^^^^^^-
    |                                                  |            |
    |                                                  |            borrowed value does not live long enough
    |                                                  argument requires that `body_reader` is borrowed for `'static`
...
465 |             }
    |             - `body_reader` dropped here while still borrowed

error[E0502]: cannot borrow `body_reader` as immutable because it is also borrowed as mutable
   --> zebra-network/src/protocol/external/codec.rs:451:22
    |
430 |                     b"tx\0\0\0\0\0\0\0\0\0\0" => self.read_tx(&mut body_reader),
    |                                                  ------------------------------
    |                                                  |            |
    |                                                  |            mutable borrow occurs here
    |                                                  argument requires that `body_reader` is borrowed for `'static`
...
451 |                 .map(|msg| {
    |                      ^^^^^ immutable borrow occurs here
...
455 |                     let extra_bytes = body.len() as u64 - body_reader.position();
    |                                                           ----------- second borrow occurs due to use of `body_reader` in closure

Some errors have detailed explanations: E0502, E0597.
For more information about an error, try `rustc --explain E0502`.
error: could not compile `zebra-network` due to 3 previous errors
warning: build failed, waiting for other jobs to finish...
error: could not compile `zebra-network` due to 3 previous errors
error: could not compile `zebra-network` due to 3 previous errors
oxarbitrage@oxarbitrage-Lenovo-ideapad-320S-14IKB:~/zebra/issue4787/zebra$ 

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found a way to make it work, I pushed it to this PR in commit 4715696.

Some of the tests will need to be changed to use the multi-threaded tokio runtime, here's an example:

#[tokio::test(flavor = "multi_thread")]
async fn connect_isolated_run_tor_multi() {

zebra-network/src/protocol/external/codec.rs Show resolved Hide resolved
@oxarbitrage oxarbitrage marked this pull request as ready for review July 21, 2022 19:42
@oxarbitrage oxarbitrage requested review from a team as code owners July 21, 2022 19:42
@oxarbitrage oxarbitrage requested review from upbqdn and removed request for a team July 21, 2022 19:42
@codecov
Copy link

codecov bot commented Jul 21, 2022

Codecov Report

Merging #4801 (06ed353) into main (81727d7) will increase coverage by 0.07%.
The diff coverage is 57.69%.

@@            Coverage Diff             @@
##             main    #4801      +/-   ##
==========================================
+ Coverage   78.75%   78.83%   +0.07%     
==========================================
  Files         305      305              
  Lines       38066    38109      +43     
==========================================
+ Hits        29980    30044      +64     
+ Misses       8086     8065      -21     

@teor2345 teor2345 added C-bug Category: This is a bug P-High 🔥 I-slow Problems with performance or responsiveness I-integration-fail Continuous integration fails, including build and test failures A-network Area: Network protocol updates or fixes A-cryptography Area: Cryptography related labels Jul 21, 2022
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good!

@teor2345 teor2345 merged commit 71fe4c4 into main Jul 21, 2022
@teor2345 teor2345 deleted the issue4787 branch July 21, 2022 23:14
@teor2345
Copy link
Contributor

I admin-merged this PR because it is an urgent sync fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cryptography Area: Cryptography related A-network Area: Network protocol updates or fixes C-bug Category: This is a bug I-integration-fail Continuous integration fails, including build and test failures I-slow Problems with performance or responsiveness
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move network transaction deserialization to a dedicated blocking and CPU-heavy thread
2 participants