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

[WIP] Wallet: Cache CWalletDB pointer in CWallet to improve performance #6966

Closed
wants to merge 1 commit into from
Closed

[WIP] Wallet: Cache CWalletDB pointer in CWallet to improve performance #6966

wants to merge 1 commit into from

Conversation

pstratem
Copy link
Contributor

@pstratem pstratem commented Nov 7, 2015

No description provided.

@pstratem pstratem changed the title Cache CWalletDB pointer in CWallet to improve performance Wallet: Cache CWalletDB pointer in CWallet to improve performance Nov 7, 2015
@pstratem pstratem changed the title Wallet: Cache CWalletDB pointer in CWallet to improve performance [WIP] Wallet: Cache CWalletDB pointer in CWallet to improve performance Nov 7, 2015
@jonasschnelli
Copy link
Contributor

Concept ACK.
Travis repots issue with rpc tests. Keeping the DB connection permanently open – I agree – is the way to go. Although it might introduce new bugs.

@pstratem
Copy link
Contributor Author

pstratem commented Nov 8, 2015

@jonasschnelli this is very very much a WIP :)

@jonasschnelli
Copy link
Contributor

Right. I almost oversaw the WIP tag.
Nice work btw!

@laanwj
Copy link
Member

laanwj commented Nov 9, 2015

Have you measured the performance improvements?

Concept ACK but I'm not convinced that this will not result in less robustness with regard to database flushes (see my other comment). This is essential, even more so for the wallet than for the utxo database.

@laanwj laanwj added Docs Wallet and removed Docs labels Nov 9, 2015
@@ -274,7 +274,7 @@ bool CWallet::ChangeWalletPassphrase(const SecureString& strOldWalletPassphrase,
return false;
if (!crypter.Encrypt(vMasterKey, pMasterKey.second.vchCryptedKey))
return false;
CWalletDB(strWalletFile).WriteMasterKey(pMasterKey.first, pMasterKey.second);
Copy link
Member

Choose a reason for hiding this comment

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

There is a reason we create CWalletDB objects every time per operation. From what I remember this assures that the transactions are closed/flushed properly.

@pstratem
Copy link
Contributor Author

pstratem commented Nov 9, 2015

@laanwj I have not specifically but "much much much faster" is how i would describe it

@laanwj
Copy link
Member

laanwj commented Nov 9, 2015

OK. That's good to hear. Just need to verify that it doesn't come at the expense of robustness, then.

@pstratem
Copy link
Contributor Author

@laanwj CDB::~CDB simply calls CDB::Close which basically just calls Flush

So it's simply a matter of ensuring that we call Flush properly

@pstratem
Copy link
Contributor Author

The easier thing to do is to simple change the default for fFlushOnClose to false, but that doesn't allow for optimizing batch operations such as keypoolrefill (which was actually my original motivation for this change).

@pstratem
Copy link
Contributor Author

Will do this more incrementally.

@pstratem pstratem closed this Nov 21, 2015
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants