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

Revert: deserialize stored transactions in a rayon thread #4933

Merged
merged 4 commits into from
Aug 30, 2022

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Aug 23, 2022

Motivation

This PR fixes a lightwalletd sync performance regression caused by PR #4801.

While that change was more correct, it also causes a 20% slowdown in lightwalletd sync performance:

  • 34 minutes to get to block 1,700,000 after this PR
  • 39-40 minutes to get to block 1,700,000 before this PR

Close #4831.

Solution

Revert the rayon threads added in PR #4801.

Review

This PR is important for some of our upcoming users, but it is not urgent.

Reviewer Checklist

  • CI passes

@teor2345 teor2345 added C-bug Category: This is a bug P-Medium ⚡ I-slow Problems with performance or responsiveness A-rpc Area: Remote Procedure Call interfaces lightwalletd any work associated with lightwalletd labels Aug 23, 2022
@teor2345 teor2345 self-assigned this Aug 23, 2022
@teor2345 teor2345 requested a review from a team as a code owner August 23, 2022 03:53
@teor2345 teor2345 requested review from upbqdn and removed request for a team August 23, 2022 03:53
@teor2345 teor2345 marked this pull request as draft August 23, 2022 03:55
@teor2345
Copy link
Contributor Author

I'm just doing a final test to make sure the speed increase wasn't due to the disk cache

@teor2345
Copy link
Contributor Author

The disk cache only saved about 45 seconds (2%), so it's still worth merging this PR.

@teor2345 teor2345 marked this pull request as ready for review August 23, 2022 04:40
@codecov
Copy link

codecov bot commented Aug 23, 2022

Codecov Report

Merging #4933 (1d87b7d) into main (1d861b0) will decrease coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #4933      +/-   ##
==========================================
- Coverage   79.04%   78.97%   -0.08%     
==========================================
  Files         309      309              
  Lines       38781    38772       -9     
==========================================
- Hits        30656    30621      -35     
- Misses       8125     8151      +26     

@teor2345
Copy link
Contributor Author

@merifyio update

@teor2345 teor2345 force-pushed the revert-state-tx-rayon branch from 487df16 to 040240e Compare August 24, 2022 00:41
@teor2345
Copy link
Contributor Author

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Aug 25, 2022

update

✅ Branch has been successfully updated

@teor2345
Copy link
Contributor Author

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Aug 28, 2022

update

✅ Branch has been successfully updated

@teor2345 teor2345 requested review from oxarbitrage and removed request for upbqdn August 29, 2022 02:07
Copy link
Contributor

@dconnolly dconnolly left a comment

Choose a reason for hiding this comment

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

lgtm

@teor2345
Copy link
Contributor Author

ssh failed to connect to Google Cloud:

kex_exchange_identification: Connection closed by remote host

This seems like a temporary error.

https://github.com/ZcashFoundation/zebra/runs/8081095982?check_suite_focus=true#step:6:110

@teor2345
Copy link
Contributor Author

@Mergifyio refresh

@mergify
Copy link
Contributor

mergify bot commented Aug 29, 2022

refresh

✅ Pull request refreshed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rpc Area: Remote Procedure Call interfaces C-bug Category: This is a bug I-slow Problems with performance or responsiveness lightwalletd any work associated with lightwalletd
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Revert deserializing state transactions in rayon threads
2 participants