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

Use NonMMappedSPVBlockStore on windows. #2403

Merged
merged 2 commits into from Feb 14, 2019

Conversation

Projects
None yet
6 participants
@oscarguindzberg
Copy link
Contributor

oscarguindzberg commented Feb 11, 2019

Fixes #2402

@ripcurlx

This comment has been minimized.

Copy link
Member

ripcurlx commented Feb 12, 2019

@devinbileck Could you give this bug fix a test?

@ripcurlx ripcurlx requested a review from devinbileck Feb 12, 2019

@@ -0,0 +1,341 @@
/*
* Copyright 2013 Google Inc.

This comment has been minimized.

@ripcurlx

ripcurlx Feb 12, 2019

Member

Is this Google copyright clause necessary at this point?

This comment has been minimized.

@oscarguindzberg

oscarguindzberg Feb 12, 2019

Author Contributor

The class is based on SPVBlockStore.java from bitcoinj that contains the copyright.
I was not sure what to do with it.
In any case, we expect to remove NonMMappedSPVBlockStore.java once we update to bitcoinj 0.15 in the next few months.

@ManfredKarrer ManfredKarrer requested a review from freimair Feb 12, 2019

@ManfredKarrer

This comment has been minimized.

Copy link
Member

ManfredKarrer commented Feb 12, 2019

@freimair Could you review that?

@devinbileck

This comment has been minimized.

Copy link
Member

devinbileck commented Feb 12, 2019

I verified this works on Windows 10!

To confirm the issue, I tested on testnet without the fix and encountered the following error:
image

With this fix, it worked and showed:
image

However, the UI became unresponsive and had lots of errors in the log. Any ideas?
bisq.log

I ended up killing the application.

But upon restart my wallet balance was restored.

@freimair
Copy link
Member

freimair left a comment

The contents of the PR seem fine to me.

As for the error log:

  • at which line of the log does the application become unresponsive (approximately)?
  • is there excessive CPU usage when the app is unresponsive?
@oscarguindzberg

This comment has been minimized.

Copy link
Contributor Author

oscarguindzberg commented Feb 13, 2019

As I commented on the PR upstream bitcoinj/bitcoinj#1695, stop using memory mapped files implies a performance degradation.

Whether the performance degradation is tiny or huge is something that has to be found out during testing. This kind of testing may be hard and confusing, because the conclusions may not be definitive.

It would be important to see if the unresponsive @devinbileck experienced is related to this PR or not.

Maybe he or someone else can do another test and see if the UI becomes unresponsive again.

@oscarguindzberg

This comment has been minimized.

Copy link
Contributor Author

oscarguindzberg commented Feb 13, 2019

About the exception seen on the @devinbileck log, I don't understand why it happened. Let's see what happens when it is tested again.

@ManfredKarrer

This comment has been minimized.

Copy link
Member

ManfredKarrer commented Feb 13, 2019

@oscarguindzberg The performance degradation is experienced always or only at restore from seed words case? We should get some rough test to see what is the difference. Maybe one other option would be to use for the receover from seed words use case a different approach and not delete the wallet file but rename and manage the new name somehow (and or rename at next restart if possible)?

@ManfredKarrer

This comment has been minimized.

Copy link
Member

ManfredKarrer commented Feb 13, 2019

@devinbileck Did you use a old wallet? If so what was the wallet date? If it is old it is expected that resync takes a lot of time and CPU. Can be a few hours if the wallet is very old and has lots of transactions.

@ManfredKarrer

This comment has been minimized.

Copy link
Member

ManfredKarrer commented Feb 13, 2019

The errors could be caused because BitcoinJ takes all CPU and then network connections drop and causing errors.

@ManfredKarrer

This comment has been minimized.

Copy link
Member

ManfredKarrer commented Feb 13, 2019

@devinbileck Could you try to run Bisq with an older wallet with that branch and see if you see any issues or performance degradation? Maybe regtest would work as well at least for a first test. So not testing the recovery use case buy just to see if there are any issues in standard use case.

@oscarguindzberg

This comment has been minimized.

Copy link
Contributor Author

oscarguindzberg commented Feb 13, 2019

@ManfredKarrer Accessing the blockchain file is expected to be slower.
Again, I don't know whether slower means a few nanoseconds or a couple of minutes. NonMMappedSPVBlockStore (and SPVBlockStore) implements a software cache, so most retrieval operations should not imply access to the blockchain file.

The performance degradation should be experienced always, but we can expect it to be worse when the blockchain file is used a lot.

During blockchain sync, the blockchain file is written a lot, so that could be an interesting point to look at for performance degradation. Blockchain sync happens in 3 cases:

  • new users
  • users who have not opened their bisq node for a while
  • after restore from seed (actually, after the user restarts her wallet as the UI suggest on restore from seed)
@schildbach

This comment has been minimized.

Copy link

schildbach commented Feb 13, 2019

I think I'd use wallet-tool to do the performance test. Something like

./wallet-tool --wallet=test.wallet create
./wallet-tool --wallet=test.wallet set-creation-time="2011/01/01"
./wallet-tool --wallet=test.wallet sync

To test again, remove the wallet file and the blockstore file and re-execute the above steps.

@oscarguindzberg

This comment has been minimized.

Copy link
Contributor Author

oscarguindzberg commented Feb 13, 2019

@schildbach probably you wanted to post that comment on bitcoinj/bitcoinj#1695 instead.

@schildbach

This comment has been minimized.

Copy link

schildbach commented Feb 13, 2019

I put this here because you guys are testing this. Thanks!

I've just added a performance test. You could probably almost copy&paste it for testing.

bitcoinj/bitcoinj@2bdc594

@ManfredKarrer

This comment has been minimized.

Copy link
Member

ManfredKarrer commented Feb 14, 2019

I did some very basic test with an older not synced app on mainnet with using NonMMappedSPVBlockStore and SPVBlockStore. 2318 blocks where downloaded.
As I am on OSX so I just changed the if switch to enable NonMMappedSPVBlockStore.

I had Bitcoin core locally to avoid too much influence from network conditions.
I run 3 times with replacing the original outdated data directory to trigger the download of all 2318 blocks:

SPVBlockStore: 51417 ms, 68965 ms, 68922 ms
NonMMappedSPVBlockStore: 92405 ms, 83738 ms, 71824 ms

So NonMMappedSPVBlockStore is a bit slower but not too critical.
Of course that is just one use case and is not sufficient to be sure if there is not more serious performance degradation, but I think it is ok to take the risk to merge that now.

I would prefer to not apply that to all OS but keep it as it is only for Windows.

@ManfredKarrer
Copy link
Member

ManfredKarrer left a comment

ACK

@ManfredKarrer ManfredKarrer merged commit 076dea2 into bisq-network:master Feb 14, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@devinbileck

This comment has been minimized.

Copy link
Member

devinbileck commented Feb 14, 2019

Sorry for the delayed response. I have tried several times on testnet and the majority of those attempts ended up with the app not responding. My wallet is from 12/6/2018.
I then tried regtest a few times and did not run into this issue.
So I am not sure it it is network related.
But for now, I agree with Manfred and should be fine to merge and get feedback from more Windows users once this fix is available and then perhaps investigate further.

@ManfredKarrer

This comment has been minimized.

Copy link
Member

ManfredKarrer commented Feb 15, 2019

@devinbileck could you try with mainnet? I could send you a data directory with the wallet I used for my tests where it was at least missing a few weeks of blocks.

@ManfredKarrer

This comment has been minimized.

Copy link
Member

ManfredKarrer commented Feb 15, 2019

Maybe also try a delete spv file to get a full resync.

@ManfredKarrer

This comment has been minimized.

Copy link
Member

ManfredKarrer commented Feb 15, 2019

I tried with that app again with resync spv file and worked all ok.

@devinbileck

This comment has been minimized.

Copy link
Member

devinbileck commented Feb 15, 2019

Just an update. I only seem to encounter the issue on testnet, not on mainnet.
I start with a fresh data dir on testnet and restore my seed (from 12/6/2018) and encounter the issue.
Perhaps it is related to the BSQ wallet?

@ManfredKarrer

This comment has been minimized.

Copy link
Member

ManfredKarrer commented Feb 15, 2019

I can reproduce an issue with the testnet directory you sent me. But it is not related to the NonMMappedSPVBlockStore as I get the same problem with or without using it.
It is only when I delete the SPV file and then it got stuck with a timeout showing the Tor settings popup. The app does not start up then even Tor is connected and BitcoinJ has all blocks. So it seems a bug in the setup handling in that case. I will work on a fix for that.

@ManfredKarrer

This comment has been minimized.

Copy link
Member

ManfredKarrer commented Feb 15, 2019

@devinbileck
In #2424 the resync issue is fixed. Could you try if you can reproduce the issue with that version?

@devinbileck

This comment has been minimized.

Copy link
Member

devinbileck commented Feb 15, 2019

@ManfredKarrer I am still encountering the issue. I wonder if the issue is that the Tor settings popup is failing to render for me. I do see this warning, but not sure if it is related:

Feb-15 11:53:51.204 [JavaFX Application Thread] WARN  b.d.m.o.popups.PopupManager: We got a isHidden called with a wrong popup.
	popup (argument)=Popup{headLine='Completed', message='Wallets restored successfully with the new seed words.

You need to shut down and restart the application.'}
	displayedPopup=Popup{headLine='Shut down in progress', message='Shutting down application can take a few seconds.
Please don't interrupt this process.'} 

bisq.log

@ManfredKarrer

This comment has been minimized.

Copy link
Member

ManfredKarrer commented Feb 15, 2019

Ah you tested seed word restore, I tested SPV file resync. Will try that as well...

@ManfredKarrer

This comment has been minimized.

Copy link
Member

ManfredKarrer commented Feb 15, 2019

I could restore the wallet but could not close the app at the end and it was under heavy load for very long time. I tested with and without the NonMMappedSPVBlockStore and both have been the same, so it seems it is an unrelated issue. I will create a new issue for investigating and fixing that.

@ManfredKarrer

This comment has been minimized.

Copy link
Member

ManfredKarrer commented Feb 15, 2019

Issue is at #2425

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.