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

[Backport] Implement accurate UTXO cache size accounting #1531

Merged
merged 6 commits into from
May 15, 2020

Conversation

furszy
Copy link

@furszy furszy commented Apr 15, 2020

This adds a basic foundation for accurate memory counting (memusage.h, with some implementation-specific assumptions, but at least the tree node and hashtable node implementations used are very generic and straightforward, so likely accurate for several systems). On the tested system at least, they are exact, ignoring memory fragmentation (tested using a single binary creating and modifying large amounts of different configurations of these data structures, and observing total resident set size afterwards). I expect this to be useful for other resource-limiting subsystems later on.

Then, this is used to implement accurate memory usage counting for CCoins objects, and efficiently computed memory usage counting for CCoinsViewCache (using cached totals, and increments/decrements on updates through CCoinsModifier). The existing CCoinsViewCache randomized simultation unit test is extended to also verify the correctness of the cached memory usage totals. Changing any of the cached total update statements in coins.cpp breaks the unit test.

Finally, the internal flush triggering mechanism is changed to use the memory usage mechanism rather than transaction count of pcoinsTip.

Coming from upstream@6102 .

--- Needs more testing ---

sipa and others added 3 commits April 14, 2020 16:40
Coming from upstream@046392dc1dd965b4ec1ba60a14a714e3e3fa7a88
Coming from upstream@fc684ad8afae19c209701230837d338c5a6c1f72
@furszy furszy self-assigned this Apr 15, 2020
@random-zebra random-zebra added this to In Progress in perpetual updating PIVX Core to BTC Core via automation Apr 15, 2020
@random-zebra random-zebra modified the milestones: Future, 5.0.0 Apr 20, 2020
@random-zebra random-zebra mentioned this pull request Apr 24, 2020
1 task
Copy link

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

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

Pretty cool. Concept and code ACK.
Will do some testing.

src/init.cpp Outdated Show resolved Hide resolved
@random-zebra
Copy link

Tested resyncing testnet with default (100 MiB) dbcache, which reserves 57.6 MiB for nCoinCacheUsage, resulted in 16 flushes to disk.

Resynced again, this time setting -dbcache=1700 (17 times the default), which translates in a 1107 MiB nCoinCacheUsage, did not result in a single database flush as expected (final pcoinsTip's cache memory usage at testnet block 1579784 is 764 MiB).

NOTE: In order to do this test, and flush only when needed (i.e. only when fCacheLarge), had to apply this patch:

Index: src/main.cpp
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- src/main.cpp	(revision 96b7ec0f6986ad609822d35bf24cfff643ac73c3)
+++ src/main.cpp	(date 1589465455694)
@@ -2741,7 +2741,7 @@
     }
     LogPrint(BCLog::BENCH, "- Disconnect block: %.2fms\n", (GetTimeMicros() - nStart) * 0.001);
     // Write the chain state to disk, if necessary.
-    if (!FlushStateToDisk(state, FLUSH_STATE_ALWAYS))
+    if (!FlushStateToDisk(state, FLUSH_STATE_IF_NEEDED))
         return false;
     // Resurrect mempool transactions from the disconnected block.
     for (const CTransaction& tx : block.vtx) {
@@ -2814,11 +2814,8 @@
     nTimeFlush += nTime4 - nTime3;
     LogPrint(BCLog::BENCH, "  - Flush: %.2fms [%.2fs]\n", (nTime4 - nTime3) * 0.001, nTimeFlush * 0.000001);
 
-    // Write the chain state to disk, if necessary. Always write to disk if this is the first of a new file.
-    FlushStateMode flushMode = FLUSH_STATE_IF_NEEDED;
-    if (pindexNew->pprev && (pindexNew->GetBlockPos().nFile != pindexNew->pprev->GetBlockPos().nFile))
-        flushMode = FLUSH_STATE_ALWAYS;
-    if (!FlushStateToDisk(state, flushMode))
+    // Write the chain state to disk, if necessary.
+    if (!FlushStateToDisk(state, FLUSH_STATE_IF_NEEDED))
         return false;
     int64_t nTime5 = GetTimeMicros();
     nTimeChainState += nTime5 - nTime4;

Not sure why we are forcing FLUSH_STATE_ALWAYS in ConnectTip (if the first of a new file: coming from #119), and in DisconnectTip. But it can be discussed/addressed in a separate PR.

Coming from upstream@b3ed4236beb7f68e1720ceb3da15e0c3682ef629
Coming from upstream@67708acff9c18e380fa6136ff0ae718959ead4b5
Make sure we're checking disk space for block index writes and allow for pruning to happen before chainstate writes.

Coming from upstream@86a5f4b54ebf5f3251f4c172cf9a5041ae43c082
@furszy furszy force-pushed the 2020_backport_upstream_6102 branch from 96b7ec0 to 9880245 Compare May 14, 2020 15:02
@furszy
Copy link
Author

furszy commented May 14, 2020

Awesome feedback and discovery @random-zebra 👌 .

Just updated the PR with the typo correction.

Copy link

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

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

ACK 9880245

perpetual updating PIVX Core to BTC Core automation moved this from In Progress to Ready May 15, 2020
Copy link
Collaborator

@Fuzzbawls Fuzzbawls left a comment

Choose a reason for hiding this comment

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

ACK 9880245

@furszy furszy merged commit 1f9e7e4 into PIVX-Project:master May 15, 2020
perpetual updating PIVX Core to BTC Core automation moved this from Ready to Done May 15, 2020
Fuzzbawls added a commit that referenced this pull request May 19, 2020
a8b965e Explicitly initialize prevector _union (random-zebra)
bdd98e8 [Trivial] Add a comment on the use of prevector in script. (random-zebra)
bbaa6e3 Clarify prevector::erase and avoid swap-to-clear (random-zebra)
28a8435 prevector: assert successful allocation (random-zebra)
de41cb5 Only call clear on prevector if it isn't trivially destructible (random-zebra)
8784df3 Make CScript (and prevector) c++11 movable. (random-zebra)
83f9ac6 serialize: Deprecate `begin_ptr` / `end_ptr` (random-zebra)
c3ecc12 prevector: add C++11-like data() method (random-zebra)
5490aa0 test prevector::swap (random-zebra)
035760e prevector::swap: fix (unreached) data corruption (random-zebra)
0e71400 prevector: destroy elements only via erase() (random-zebra)
9811a68 [Core] Prevector type (random-zebra)

Pull request description:

  Based on:
  - [x] #1554

  This introduces `prevector<N, T>`, a new basic data type which is a fully API compatible drop-in replacement for `std::vector<T>` and uses it for the script, to reduce the considerable memory overhead of vectors in cases where they normally contain a small number of small elements.

  Original tests in Bitcoin showed a reduction in dbcache memory usage by **23%**, and made an initial sync **13%** faster.

  Backported from the following upstream's PRs, with only additional edits in the 2nd layer scripts (`masternode-payments`, `masternode-budget`) and in the unit tests (to address the differences in `insecure_rand`):

  - bitcoin#6914
  - bitcoin#7888
  - bitcoin#8850
  - bitcoin#9349
  - bitcoin#9505
  - bitcoin#9856
  - bitcoin#10534
  - bitcoin#11011
  - bitcoin#14028

  ~~NOTE: Updates to `memusage.h` needed as well, when #1531 is merged.~~

ACKs for top commit:
  furszy:
    Tested ACK a8b965e .
  Fuzzbawls:
    ACK a8b965e

Tree-SHA512: 203b660dd8d9f98780be660dae0ac951766ba438836bd6f0ec921e7749c1a84143f3b920fe49f2cc399f4aa7e763098f78746d3b6ff845bcf913bb13de984bc1
random-zebra added a commit that referenced this pull request Jun 3, 2020
368665e Implement accurate memory accounting for mempool (random-zebra)

Pull request description:

  Based on top of:
  - [x] #1641

  This continues the work of #1531 adding memory accounting for the mempool.
  Backports bitcoin#6410.

  Original description:
  > This implements accurate memory usage accounting for the mempool. It is only exposed through getmempoolinfo for now, but could be used for limiting the resource requirements too (bitcoin#6281).

ACKs for top commit:
  furszy:
    pretty nice, tested ACK 368665e
  Fuzzbawls:
    ACK 368665e

Tree-SHA512: f1dd0e98af58133255db02ae57f20c5d1c0b210610bf6e6c99a112c8c74c0e83e0ae05fd22a933cc4db0eaca36b5f45fa27231879809b348ba0dba034e176767
@furszy furszy deleted the 2020_backport_upstream_6102 branch November 29, 2022 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants