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.

Sign up for GitHub

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

Jump to bottom

Pipeline requests near the chain tip too, with care #1665

Merged
merged 3 commits into from
May 15, 2020
Merged

Pipeline requests near the chain tip too, with care #1665

merged 3 commits into from
May 15, 2020

Conversation

KtorZ
Copy link
Member

@KtorZ KtorZ commented May 14, 2020 •
edited
Loading

Issue Number

#1650

Overview

  • 0a90f23
    📍 Pipeline requests near the chain tip too, with care
    This is a bit tricky because, as soon as we enter the tip - k area,
    any 'RequestNext' may yield a 'RollBackward'. The strategy is however
    quite simple and boils down to two cases:

    • Either we have to rollback within blocks we have just collected but
      not yet sent for processing.

    • Either, we have to rollback beyond this.

    In the former case, we can simply discard blocks from what we have
    collected and continue as normal. In the latter, we have to forward
    the rollback requests to the wallet engine, but we have to keep on
    collecting the results for the requests we have already pipelined!

    We do so by stashing the results in a queue, so that they'd be
    available for the next 'RequestNext' command from the wallet.

    This effectively extend pipelining up to the point where we have
    caught up with the tip and start collecting requests one by one, in a
    calmly manner. This is visible from the logs:

    ...
Applying blocks [192.13732 ... 192.14731]
Applying blocks [192.14732 ... 192.15731]
Applying blocks [192.15732 ... 192.15977]
Applying blocks [192.15978 ... 192.15978]
Applying blocks [192.15979 ... 192.15979]
Applying blocks [192.15980 ... 192.15980]

Comments

This should fix #1650

@KtorZ KtorZ requested review from Anviking and piotr-iohk May 14, 2020 14:48
@KtorZ KtorZ self-assigned this May 14, 2020
Anviking
Anviking reviewed May 14, 2020
View reviewed changes
lib/byron/1-percent.timelog Outdated
1589384183.268922344s -0
1589384183.34260801s 1000
1589384183.401558201s 2000
1589384183.462808633s 3000
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't be committed!

Copy link
Contributor

Choose a reason for hiding this comment

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

That's for boosting GH statistics ;P

Screenshot from 2020-05-14 16-53-04

Copy link
Member Author

Choose a reason for hiding this comment

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

I might have abused git add lib 😄 ...

@KtorZ
Pipeline requests near the chain tip too, with care
f4d26b4 
  This is a bit tricky because, as soon as we enter the `tip - k` area,
  any 'RequestNext' may yield a 'RollBackward'. The strategy is however
  quite simple and boils down to two cases:

  - Either we have to rollback within blocks we have just collected but
    not yet sent for processing.

  - Either, we have to rollback beyond this.

  In the former case, we can simply discard blocks from what we have
  collected and continue as normal. In the latter, we have to forward
  the rollback requests to the wallet engine, but we have to keep on
  collecting the results for the requests we have already pipelined!

  We do so by stashing the results in a queue, so that they'd be
  available for the next 'RequestNext' command from the wallet.

  This effectively extend pipelining up to the point where we have
  caught up with the tip and start collecting requests one by one, in a
  calmly manner. This is visible from the logs:

  ```
  ...
  Applying blocks [192.13732 ... 192.14731]
  Applying blocks [192.14732 ... 192.15731]
  Applying blocks [192.15732 ... 192.15977]
  Applying blocks [192.15978 ... 192.15978]
  Applying blocks [192.15979 ... 192.15979]
  Applying blocks [192.15980 ... 192.15980]
  ```
@KtorZ KtorZ force-pushed the KtorZ/pipeline-in-unstable-zone branch from bf05724 to f4d26b4 Compare May 14, 2020 15:41
Anviking
Anviking approved these changes May 15, 2020
View reviewed changes
Copy link
Member

@Anviking Anviking left a comment

Choose a reason for hiding this comment

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

lgtm.
I guess testing that everything works if the chain forks in the middle of collecting responses, etc, would be nice, but not sure how one would do that.

lib/core/src/Ouroboros/Network/Client/Wallet.hs
-- receiving future requests, we'll simply read them from the stash!
, P.recvMsgRollBackward = \point _tip ->
case rollback point blocks of
[] -> do -- b)
Copy link
Member

Choose a reason for hiding this comment

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

It probably doesn't matter, but I think you can't differentiate between:

  • Rolling back to the latest persisted block ([], but no respond (Rollback point) needed)
  • Rolling back to one before the latest persisted block (rollback needed)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this would induce a rollback to where the wallet already is at, which is a no-op for the wallet and database. So, in principle, there's nothing wrong from it. Yet indeed, the client is blind to what the latest persisted block is so it can't really make this distinction.

rvl
rvl approved these changes May 15, 2020
View reviewed changes
Copy link
Contributor

@rvl rvl left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Would the design be simpler if we changed network layer from pull (nextBlocks) to push (onNextBlock)?

It would also be good to allocate some time to unit testing the cardano-node network layers because they are not totally simple any more.

lib/byron/src/Cardano/Wallet/Byron/Network.hs Outdated
nodeToClientProtocols NodeToClientProtocols
-> m (NetworkClient m)
mkWalletClient bp chainSyncQ = do
stash <- atomically newTQueue
Copy link
Contributor

Choose a reason for hiding this comment

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

This could maybe be called "responseBuffer"

Copy link
Member Author

Choose a reason for hiding this comment

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

done in: 0446953 👍

@KtorZ
Copy link
Member Author

KtorZ commented May 15, 2020

Would the design be simpler if we changed network layer from pull (nextBlocks) to push (onNextBlock)?

Yes, having the buffering done in the network clients is awkward and it seems more and more suggesting that a push approach would be better suited. Johannes also suggested that a few times; I am still worried about the amount of work this would require on both cardano-node + jormungandr :/

@KtorZ
Copy link
Member Author

KtorZ commented May 15, 2020

I guess testing that everything works if the chain forks in the middle of collecting responses, etc, would be nice, but not sure how one would do that.
It would also be good to allocate some time to unit testing the cardano-node network layers because they are not totally simple any more.

Yes, in particular, the behavior on roll backs is non trivial :(

@KtorZ KtorZ added the IMPROVEMENT Mark a PR as an improvement, for auto-generated CHANGELOG label May 15, 2020
@KtorZ
Copy link
Member Author

KtorZ commented May 15, 2020

bors r+

iohk-bors bot added a commit that referenced this pull request May 15, 2020
@iohk-bors @KtorZ
Merge #1665
0f929c4 
1665: Pipeline requests near the chain tip too, with care r=KtorZ a=KtorZ

# Issue Number

<!-- Put here a reference to the issue this PR relates to and which requirements it tackles -->

#1650 


# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- 0a90f23
  📍 **Pipeline requests near the chain tip too, with care**
    This is a bit tricky because, as soon as we enter the `tip - k` area,
  any 'RequestNext' may yield a 'RollBackward'. The strategy is however
  quite simple and boils down to two cases:

  - Either we have to rollback within blocks we have just collected but
    not yet sent for processing.

  - Either, we have to rollback beyond this.

  In the former case, we can simply discard blocks from what we have
  collected and continue as normal. In the latter, we have to forward
  the rollback requests to the wallet engine, but we have to keep on
  collecting the results for the requests we have already pipelined!

  We do so by stashing the results in a queue, so that they'd be
  available for the next 'RequestNext' command from the wallet.

  This effectively extend pipelining up to the point where we have
  caught up with the tip and start collecting requests one by one, in a
  calmly manner. This is visible from the logs:

  ```
  ...
  Applying blocks [192.13732 ... 192.14731]
  Applying blocks [192.14732 ... 192.15731]
  Applying blocks [192.15732 ... 192.15977]
  Applying blocks [192.15978 ... 192.15978]
  Applying blocks [192.15979 ... 192.15979]
  Applying blocks [192.15980 ... 192.15980]
  ```


# Comments

<!-- Additional comments or screenshots to attach if any -->

This should fix #1650 

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Assign the PR to a corresponding milestone
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: KtorZ <[email protected]>
Co-authored-by: IOHK <[email protected]>
@piotr-iohk piotr-iohk mentioned this pull request May 15, 2020
Closed
@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 15, 2020

Timed out

@KtorZ
Copy link
Member Author

KtorZ commented May 15, 2020 •
edited
Loading

Weird, the build succeeded but somehow bors never reported to github.

https://buildkite.com/input-output-hk/cardano-wallet/builds/7548

@KtorZ KtorZ merged commit 0bf1099 into master May 15, 2020
@KtorZ KtorZ deleted the KtorZ/pipeline-in-unstable-zone branch May 15, 2020 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rvl rvl rvl approved these changes

@Anviking Anviking Anviking approved these changes

@piotr-iohk piotr-iohk Awaiting requested review from piotr-iohk

Assignees

@KtorZ KtorZ

Labels
IMPROVEMENT Mark a PR as an improvement, for auto-generated CHANGELOG
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Restoration benchmark on mainnet for 1% and 2% ownership unexpectedly slow
4 participants
@KtorZ @Anviking @rvl @piotr-iohk