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

1. 
   luke-jr commented at 9:24 AM on October 19, 2015: member

   Huge performance improvement (450%) for zapwallettxes.

   Also improves performance for RPC listtransactions.

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

2. 
   laanwj commented at 9:25 AM on October 19, 2015: member

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

3. laanwj added the label Wallet on Oct 19, 2015
4. luke-jr force-pushed on Oct 19, 2015
5. 
   luke-jr commented at 9:32 AM on October 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.

6. 
   luke-jr commented at 4:55 AM on October 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
   

7. in src/wallet/walletdb.cpp:None in 5873cc890d outdated

    188 | @@ -189,7 +189,7 @@ bool CWalletDB::WriteAccountingEntry(const uint64_t nAccEntryNum, const CAccount
    189 |      return Write(std::make_pair(std::string("acentry"), std::make_pair(acentry.strAccount, nAccEntryNum)), acentry);
    190 |  }
    191 |  
    192 | -bool CWalletDB::WriteAccountingEntry(const CAccountingEntry& acentry)
    193 | +bool CWalletDB::WriteAccountingEntry_ForWalletOnly(const CAccountingEntry& acentry)
   

   ---

   ---

   

   jonasschnelli commented at 7:06 PM on October 29, 2015:

   nit: use a better method name? Maybe

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

8. 
   jonasschnelli commented at 7:12 PM on October 29, 2015: contributor

   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.

9. 
   dcousens commented at 7:19 PM on October 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.

10. gmaxwell added this to the milestone 0.12.0 on Nov 5, 2015
11. 
    fanquake commented at 4:23 AM on November 6, 2015: member

    Needs rebase

12. 
    gmaxwell commented at 7:57 AM on November 14, 2015: contributor

    @luke-jr rebase please

13. luke-jr force-pushed on Nov 17, 2015
14. 
    luke-jr commented at 4:38 PM on November 17, 2015: member

    Rebased

15. 
    pstratem commented at 9:46 PM on November 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 }

16. 
    bittylicious commented at 10:05 PM on November 18, 2015: none

    @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.

17. 
    gmaxwell commented at 10:43 PM on November 18, 2015: contributor

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

18. 
    gmaxwell commented at 12:04 AM on November 21, 2015: contributor

    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.

19. 
    gmaxwell commented at 12:44 AM on November 21, 2015: contributor

    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.

20. 
    bittylicious commented at 1:01 AM on November 21, 2015: none

    @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.

21. luke-jr force-pushed on Nov 21, 2015
22. Optimisation: Store transaction list order in memory rather than compute it every need

    Huge performance improvement (450%) for zapwallettxes

    3e7c89196c
23. luke-jr force-pushed on Nov 21, 2015
24. 
    gmaxwell commented at 2:53 AM on November 21, 2015: contributor

    ACK.

25. gmaxwell merged this on Nov 21, 2015
26. gmaxwell closed this on Nov 21, 2015

    ---

27. gmaxwell referenced this in commit 616d61b20d on Nov 21, 2015
28. zkbot referenced this in commit 5b194067ea on Aug 12, 2021
29. MarcoFalke locked this on Sep 8, 2021

Milestone
0.12.0