Optimisation: Store transaction list order in memory rather than compute it every need #6851

Merged
merged 1 commit into from Nov 21, 2015

Conversation

Projects
None yet
8 participants
@luke-jr
Member

luke-jr commented Oct 19, 2015

Huge performance improvement (450%) for zapwallettxes.

Also improves performance for RPC listtransactions.

(For comparison, removing smart-time entirely is only 340%)

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Oct 19, 2015

Member

People are already complaining about memory usage of the wallet. How much will this make it worse?

Member

laanwj commented Oct 19, 2015

People are already complaining about memory usage of the wallet. How much will this make it worse?

@laanwj laanwj added the Wallet label Oct 19, 2015

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Oct 19, 2015

Member

I would guess maybe 40 bytes per transaction. I am running some tests to compare.

The current code is very poor for zapwallettxes because it will re-sort the entire wallet transactions, every time it adds a new one.

Member

luke-jr commented Oct 19, 2015

I would guess maybe 40 bytes per transaction. I am running some tests to compare.

The current code is very poor for zapwallettxes because it will re-sort the entire wallet transactions, every time it adds a new one.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Oct 20, 2015

Member

With a wallet containing 10184 transactions, I did not observe any measurable memory overhead to the cache.

 SIZE    VSZ    SZ
329352 347192 86798 d78a880
320552 338396 84599 d78a880+5873cc8
Member

luke-jr commented Oct 20, 2015

With a wallet containing 10184 transactions, I did not observe any measurable memory overhead to the cache.

 SIZE    VSZ    SZ
329352 347192 86798 d78a880
320552 338396 84599 d78a880+5873cc8
@jonasschnelli

View changes

src/wallet/walletdb.cpp
@@ -189,7 +189,7 @@ bool CWalletDB::WriteAccountingEntry(const uint64_t nAccEntryNum, const CAccount
return Write(std::make_pair(std::string("acentry"), std::make_pair(acentry.strAccount, nAccEntryNum)), acentry);
}
-bool CWalletDB::WriteAccountingEntry(const CAccountingEntry& acentry)
+bool CWalletDB::WriteAccountingEntry_ForWalletOnly(const CAccountingEntry& acentry)

This comment has been minimized.

@jonasschnelli

jonasschnelli Oct 29, 2015

Member

nit: use a better method name? Maybe

_WriteAccountingEntry(const CAccountingEntry& acentry) //should only be called from the CWallet
@jonasschnelli

jonasschnelli Oct 29, 2015

Member

nit: use a better method name? Maybe

_WriteAccountingEntry(const CAccountingEntry& acentry) //should only be called from the CWallet
@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Oct 29, 2015

Member

Nice!
Adding a wtx to the wallet could also be faster (especially big wallets) because AddToWallet() does also call OrderedTxItems() (which reads in all the whole transaction history).

utACK.

Member

jonasschnelli commented Oct 29, 2015

Nice!
Adding a wtx to the wallet could also be faster (especially big wallets) because AddToWallet() does also call OrderedTxItems() (which reads in all the whole transaction history).

utACK.

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Oct 29, 2015

Contributor

concept ACK, light utACK

Also, I feel like any memory overhead required would already exist instantaneously at the call site anyway, this just persists the requirement for the lifetime of the application instead?

edit: To clarify, this doesn't really require any extra memory, except in that it is now persistent across the applications lifetime, not just when needed.

Contributor

dcousens commented Oct 29, 2015

concept ACK, light utACK

Also, I feel like any memory overhead required would already exist instantaneously at the call site anyway, this just persists the requirement for the lifetime of the application instead?

edit: To clarify, this doesn't really require any extra memory, except in that it is now persistent across the applications lifetime, not just when needed.

@gmaxwell gmaxwell added this to the 0.12.0 milestone Nov 5, 2015

@fanquake

This comment has been minimized.

Show comment
Hide comment
@fanquake

fanquake Nov 6, 2015

Member

Needs rebase

Member

fanquake commented Nov 6, 2015

Needs rebase

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Nov 14, 2015

Member

@luke-jr rebase please

Member

gmaxwell commented Nov 14, 2015

@luke-jr rebase please

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Nov 17, 2015

Member

Rebased

Member

luke-jr commented Nov 17, 2015

Rebased

@pstratem

This comment has been minimized.

Show comment
Hide comment
@pstratem

pstratem Nov 18, 2015

Contributor

Increased memory from 1.5GB on master to 1.8GB
Improved zapwallet time to 5 minutes from something approximating infinity

$ ./src/bitcoin-cli -testnet getwalletinfo
{
"walletversion": 60000,
"balance": 1937898.88787003,
"unconfirmed_balance": 0.00000000,
"immature_balance": 1275.53301591,
"txcount": 776267,
"keypoololdest": 1446285387,
"keypoolsize": 999999,
"paytxfee": 0.00000000
}

Contributor

pstratem commented Nov 18, 2015

Increased memory from 1.5GB on master to 1.8GB
Improved zapwallet time to 5 minutes from something approximating infinity

$ ./src/bitcoin-cli -testnet getwalletinfo
{
"walletversion": 60000,
"balance": 1937898.88787003,
"unconfirmed_balance": 0.00000000,
"immature_balance": 1275.53301591,
"txcount": 776267,
"keypoololdest": 1446285387,
"keypoolsize": 999999,
"paytxfee": 0.00000000
}

@bittylicious

This comment has been minimized.

Show comment
Hide comment
@bittylicious

bittylicious Nov 18, 2015

@pstratem Pull request 6850 will improve zapwallet / rescan time similarly with no memory increase. However, this pull request will be good for AddToWallets when near the tip, where there is intermittent CPU spiking when the indexes are recreated.

IMO, this pull request is needed even with the memory increase, but 6850 is less controversial.

@pstratem Pull request 6850 will improve zapwallet / rescan time similarly with no memory increase. However, this pull request will be good for AddToWallets when near the tip, where there is intermittent CPU spiking when the indexes are recreated.

IMO, this pull request is needed even with the memory increase, but 6850 is less controversial.

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Nov 18, 2015

Member

Rescan on wallet containing 14WCrzmqVCD7Thf79vutWTB6y1hUc8pP19 (maybe about 53k transactions) went from 23 minutes to 10 minutes.

Member

gmaxwell commented Nov 18, 2015

Rescan on wallet containing 14WCrzmqVCD7Thf79vutWTB6y1hUc8pP19 (maybe about 53k transactions) went from 23 minutes to 10 minutes.

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Nov 21, 2015

Member

oh my numbers there were for the whole rescan, if I limit the benchmark to just the chunk of the blockchain that have transactions... it goes from 843 seconds to 35 seconds from 63/sec to 1541/sec; or 24x faster.

Member

gmaxwell commented Nov 21, 2015

oh my numbers there were for the whole rescan, if I limit the benchmark to just the chunk of the blockchain that have transactions... it goes from 843 seconds to 35 seconds from 63/sec to 1541/sec; or 24x faster.

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Nov 21, 2015

Member

On a wallet with 410k transactions in it, this repeatably decreases memory usage by ~5MB from 822MB to 816MB. I think it's fair to say that any increase is negligible.

Member

gmaxwell commented Nov 21, 2015

On a wallet with 410k transactions in it, this repeatably decreases memory usage by ~5MB from 822MB to 816MB. I think it's fair to say that any increase is negligible.

@bittylicious

This comment has been minimized.

Show comment
Hide comment
@bittylicious

bittylicious Nov 21, 2015

@gmaxwell I would expect any memory increase to be within "measurement error" tolerances, i.e. not significant. This is essentially just storing another index (or two?). It's great that your measurements seem to agree.

And the previous code which ultimately recreated the indexes on each AddToWallet is simply bad.

@gmaxwell I would expect any memory increase to be within "measurement error" tolerances, i.e. not significant. This is essentially just storing another index (or two?). It's great that your measurements seem to agree.

And the previous code which ultimately recreated the indexes on each AddToWallet is simply bad.

Optimisation: Store transaction list order in memory rather than comp…
…ute it every need

Huge performance improvement (450%) for zapwallettxes
@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Nov 21, 2015

Member

ACK.

Member

gmaxwell commented Nov 21, 2015

ACK.

@gmaxwell gmaxwell merged commit 3e7c891 into bitcoin:master Nov 21, 2015

1 check passed

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

gmaxwell added a commit that referenced this pull request Nov 21, 2015

Merge pull request #6851
3e7c891 Optimisation: Store transaction list order in memory rather than compute it every need (Luke Dashjr)

@str4d str4d referenced this pull request in zcash/zcash Mar 29, 2017

Open

Bitcoin 0.12 wallet PRs 1 #2225

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment