zcash backport of memory optimizations by Alrighttt · Pull Request #582 · GLEECBTC/komodo-daemon

Alrighttt · 2023-03-31T14:47:05Z

Port of zcash#6192

Have asked some community members to test this code, and we are seeing significant reduction in RAM usage.

It should reduce the RAM usage by about 1344*height. See the zcash PR for technical details.

My komodo node with all indexes enabled was using 7.2GB of RAM. With this patch applied, it now uses ~2.8GB.

dimxy · 2023-03-31T16:43:02Z

Good to know how this will affect sync performance (as getheaders now involves hdd read)

DeckerSU · 2023-03-31T18:28:56Z

src/test-komodo/test_haraka_removal.cpp

        EXPECT_EQ(0, chainActive.Height());

-        CDiskBlockIndex diskindex {&fakeIndex};
+        CDiskBlockIndex diskindex(&fakeIndex, [](){ return std::vector<unsigned char>(); });


Part of this test that serializes CDiskBlockIndex should be commented / removed, like:

/* CDiskBlockIndex diskindex {&fakeIndex}; ss.clear(); ss << diskindex; // VARINT(PROTOCOL_VERSION) - 89af1a static constexpr const char* stream_genesis_block_diskindex_hex = "89af1a00000000000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000000000000000003ba3edfd7a7b12b27ac72c3e67768f617fc81bc3888a51323a9fb8aa4b1e5e4a000000000000000000000000000000000000000000000000000000000000000029ab5f490f0f0f200b00000000000000000000000000000000000000000000000000000000000000fd4005000d5ba7cda5d473947263bf194285317179d2b0d307119c2e7cc4bd8ac456f0774bd52b0cd9249be9d40718b6397a4c7bbd8f2b3272fed2823cd2af4bd1632200ba4bf796727d6347b225f670f292343274cc35099466f5fb5f0cd1c105121b28213d15db2ed7bdba490b4cedc69742a57b7c25af24485e523aadbb77a0144fc76f79ef73bd8530d42b9f3b9bed1c135ad1fe152923fafe98f95f76f1615e64c4abb1137f4c31b218ba2782bc15534788dda2cc08a0ee2987c8b27ff41bd4e31cd5fb5643dfe862c9a02ca9f90c8c51a6671d681d04ad47e4b53b1518d4befafefe8cadfb912f3d03051b1efbf1dfe37b56e93a741d8dfd80d576ca250bee55fab1311fc7b3255977558cdda6f7d6f875306e43a14413facdaed2f46093e0ef1e8f8a963e1632dcbeebd8e49fd16b57d49b08f9762de89157c65233f60c8e38a1f503a48c555f8ec45dedecd574a37601323c27be597b956343107f8bd80f3a925afaf30811df83c402116bb9c1e5231c70fff899a7c82f73c902ba54da53cc459b7bf1113db65cc8f6914d3618560ea69abd13658fa7b6af92d374d6eca9529f8bd565166e4fcbf2a8dfb3c9b69539d4d2ee2e9321b85b331925df195915f2757637c2805e1d4131e1ad9ef9bc1bb1c732d8dba4738716d351ab30c996c8657bab39567ee3b29c6d054b711495c0d52e1cd5d8e55b4f0f0325b97369280755b46a02afd54be4ddd9f77c22272b8bbb17ff5118fedbae2564524e797bd28b5f74f7079d532ccc059807989f94d267f47e724b3f1ecfe00ec9e6541c961080d8891251b84b4480bc292f6a180bea089fef5bbda56e1e41390d7c0e85ba0ef530f7177413481a226465a36ef6afe1e2bca69d2078712b3912bba1a99b1fbff0d355d6ffe726d2bb6fbc103c4ac5756e5bee6e47e17424ebcbf1b63d8cb90ce2e40198b4f4198689daea254307e52a25562f4c1455340f0ffeb10f9d8e914775e37d0edca019fb1b9c6ef81255ed86bc51c5391e0591480f66e2d88c5f4fd7277697968656a9b113ab97f874fdd5f2465e5559533e01ba13ef4a8f7a21d02c30c8ded68e8c54603ab9c8084ef6d9eb4e92c75b078539e2ae786ebab6dab73a09e0aa9ac575bcefb29e930ae656e58bcb513f7e3c17e079dce4f05b5dbc18c2a872b22509740ebe6a3903e00ad1abc55076441862643f93606e3dc35e8d9f2caef3ee6be14d513b2e062b21d0061de3bd56881713a1a5c17f5ace05e1ec09da53f99442df175a49bd154aa96e4949decd52fed79ccf7ccbce32941419c314e374e4a396ac553e17b5340336a1a25c22f9e42a243ba5404450b650acfc826a6e432971ace776e15719515e1634ceb9a4a35061b668c74998d3dfb5827f6238ec015377e6f9c94f38108768cf6e5c8b132e0303fb5a200368f845ad9d46343035a6ff94031df8d8309415bb3f6cd5ede9c135fdabcc030599858d803c0f85be7661c88984d88faa3d26fb0e9aac0056a53f1b5d0baed713c853c4a2726869a0a124a8a5bbc0fc0ef80c8ae4cb53636aa02503b86a1eb9836fcc259823e2692d921d88e1ffc1e6cb2bde43939ceb3f32a611686f539f8f7c9f0bf00381f743607d40960f06d347d1cd8ac8a51969c25e37150efdf7aa4c2037a2fd0516fb444525ab157a0ed0a7412b2fa69b217fe397263153782c0f64351fbdf2678fa0dc8569912dcd8e3ccad38f34f23bbbce14c6a26ac24911b308b82c7e43062d180baeac4ba7153858365c72c63dcf5f6a5b08070b730adb017aeae925b7d0439979e2679f45ed2f25a7edcfd2fb77a8794630285ccb0a071f5cce410b46dbf9750b0354aae8b65574501cc69efb5b6a43444074fee116641bb29da56c2b4a7f456991fc92b2"; EXPECT_EQ(HexStr(ss), stream_genesis_block_diskindex_hex); */

Bcz it has protocol version involved, which can be different on next release of komodod.

I initially had this fully commented, but I was unsure what the purpose of it. Could we remove this file entirely?

I initially had this fully commented, but I was unsure what the purpose of it. Could we remove this file entirely?

I don't think so, we can safely remove exactly mentioned above part of test_block_ser test, but what about others - till we have -ac_timeunlock, -ac_timeunlockfrom better to keep test_block_prg exist.

As a cross-reference, I made a similar PR in KomodoOcean repo: DeckerSU/KomodoOcean#49 , difference is in that I cherry-picked needed commits from the upstream, so it preserves the authors, etc. Now I have plan to add metrics / probably using Prometheus as in ZCash, to count / measure / estimate the count / frequency of WriteBatchSync calls, mean, how much times index entries are rewritten, how much times nSolution are read from disk, how much disk load has increased with these changes, etc. Also, I want to assess necessity of caching reading nSolution from block index DB, etc. May be it's worth of use something like lrucache, mruset, etc., for the often requested solutions. For example, like for 1000 "last" requested solutions / blocks, etc.

DeckerSU · 2023-04-01T00:19:21Z

Also I'll recommend to add these 3 commits analogs as a part of original zcash#6192 PR:

DeckerSU/KomodoOcean@f7d014d - Error reporting improvements.
DeckerSU/KomodoOcean@52334a6 - Improve handling of database read errors.
DeckerSU/KomodoOcean@1f981a8 - Declare CBlockTreeDB::Read* methods as const when they are trivially so.

Not so critical, of course, but could save time in case of errors investigation, etc.

dimxy · 2023-04-01T10:17:47Z

May be it's worth of use something like lrucache, mruset, etc., for the often requested solutions.

In the zcash discussion it is stated that there is no need for caching as leveldb does this itself. Maybe it also has some settings and we could consider changing them for this fix

DeckerSU · 2023-04-01T11:10:36Z

May be it's worth of use something like lrucache, mruset, etc., for the often requested solutions.

In the zcash discussion it is stated that there is no need for caching as leveldb does this itself. Maybe it also has some settings and we could consider changing them for this fix

Yes, I already read this. Of course it have settings, you can change them by -dbcache command line argument, but anyway I need to do my own "investigation" to better understand the things.

dimxy · 2023-04-05T17:37:06Z

I did a full sync (on the same dedicated PC) between two nodes, using 2 nodes both with no and with this fix, and did not see that the fix slowed down the sync:
without the fix the sync time was 17 hours 15 minutes
with the fix the sync time was 16 hours 9 minutes (surprisingly)

DeckerSU · 2023-04-06T00:59:11Z

After analyzing the code, I came to the conclusions which I wrote in this issue: zcash#6532. If I am wrong somewhere, please point it out to me.

p.s. @Alrighttt: Do you plan to add the commits mentioned there #582 (comment), or do you want me to add it to your branch myself?

dimxy · 2023-04-06T08:22:10Z

I came to the conclusions which I wrote in this issue: zcash#6532.

Is this issue relevant only to zcash only or to komodo too?

DeckerSU · 2023-04-06T10:15:12Z

Is this issue relevant only to zcash only or to komodo too?

Both. As both have these consistency checks.

dimxy · 2023-05-18T13:24:19Z

Are we going to add more fixes to this PR?
it's good to merge it before the annual HF

DeckerSU · 2023-05-25T15:58:24Z

@dimxy what we will do with error handling changes mentioned in #582 (comment) ? As I see @Alrighttt still didn't add them, so we have few options:

Add these changes into Alrighttt:zcash-6192-backport branch before merge this PR.
Add these changes after merge these PR in our KomodoPlatform:dev branch.
Don't add them at all (not recommended, from my point of view).

Which way shall we go?

p.s. I also highly appreciate merging these changes before the annual HF. As well as these perfomance improvements, mentioned in:

I can handle the consistency checks removal PR.

dimxy · 2023-05-25T19:14:19Z

I suggest that you add your improvements to this PR and Alright merge them, and then we merge this PR to dev branch

- DeckerSU/KomodoOcean@f7d014d - DeckerSU/KomodoOcean@52334a6

DeckerSU/KomodoOcean@1f981a8

Details: DeckerSU/KomodoOcean@24eaf1e

DeckerSU · 2023-05-25T21:08:52Z

I suggest that you add your improvements to this PR and Alright merge them, and then we merge this PR to dev branch

Done in Alrighttt#42 . This PR contains changes mentioned here #582 (comment) + backport of DeckerSU/KomodoOcean@24eaf1e , discussed earlier.

So, now we are waiting for merging Alrighttt#42 -> https://github.com/Alrighttt/komodo/tree/zcash-6192-backport, and then we should re-review, approve and merge this #582 into dev.

cc: @Alrighttt

error handling improvements + get rid of consistency checks

dimxy · 2023-05-30T18:13:00Z

src/chain.h

+    //! This method should not be called on a CDiskBlockIndex.
+    void TrimSolution()
+    {
+        assert(!"called CDiskBlockIndex::TrimSolution");


This is a bit strange.
Why would we need yet another TrimSolution() which just fails, isn't it better not to have it at all and prevent builds with such calls?

I guess it's bcz CDiskBlockIndex inherited from CBlockIndex, which is already have TrimSolution() method. To prevent TrimSolution() calls for CDiskBlockIndex instances, it was declared as private. And method body contains assertion to emphasize that this method is not allowed to be called for CDiskBlockIndex.

The main purpose of this is to prevent inheritance TrimSolution() implementation from CBlockIndex, which contains real code for clear nSolution vector. Of course we could do it like:

class CDiskBlockIndex : public CBlockIndex { // ... void TrimSolution() = delete; // ... };

But I guess original implementation a little clearer. Also, it's a just a backport of zcash#6192, so I guess better to follow upstream here.

I guess 'private' is enough but that's okay

dimxy · 2023-05-30T19:32:11Z

I thought yet we could replace other calls to nSolution.clear() with the proper memory cleanup like:

std::vector<unsigned char> empty;
nSolution.swap(empty);

F.e. komodo_blockload() loads blocks from HDD and then invokes nSolution.clear() via block.SetNull() not actually cleaning the memory. However it looks like all such invocations are applied to stack vars so they are cleaned automatically and this fix seems to be not necessary.

src/txdb.cpp

dimxy · 2023-05-30T20:06:17Z

LGTM

Release v1.2.3-6

Alrighttt and others added 5 commits March 30, 2023 10:16

backport zcash PR6192 memory optimizations

76213ce

update copyright & fix nspv nsolution

9156f90

latest zcash blockindex Write/Erase

ad5065f

temporarily disable haraka tests

bb595ac

fix test haraka

3aa80e6

DeckerSU requested review from DeckerSU and dimxy March 31, 2023 14:55

DeckerSU reviewed Mar 31, 2023

View reviewed changes

DeckerSU mentioned this pull request Mar 31, 2023

Reduce memory usage of CBlockIndex DeckerSU/KomodoOcean#49

Merged

2 tasks

DeckerSU added 3 commits May 25, 2023 22:11

Error handling improvements.

46c237a

- DeckerSU/KomodoOcean@f7d014d - DeckerSU/KomodoOcean@52334a6

Declare CBlockTreeDB::Read* methods as const when they are trivially so.

3115f20

DeckerSU/KomodoOcean@1f981a8

txdb: get rid of consistency checks

688de60

Details: DeckerSU/KomodoOcean@24eaf1e

Merge pull request #42 from DeckerSU/patch-add-error-handling

bf0b713

error handling improvements + get rid of consistency checks

DeckerSU requested a review from ca333 May 27, 2023 10:18

DeckerSU mentioned this pull request May 30, 2023

[release] v0.8.0 #586

Merged

dimxy reviewed May 30, 2023

View reviewed changes

src/txdb.cpp Show resolved Hide resolved

ca333 approved these changes May 31, 2023

View reviewed changes

ca333 merged commit 4651e28 into GLEECBTC:dev May 31, 2023

DeckerSU mentioned this pull request Jun 12, 2023

Reduce memory usage of CBlockIndex + Build fixes marmarachain/marmara#38

Merged

who-biz pushed a commit to who-biz/komodo that referenced this pull request Aug 18, 2024

Merge pull request GLEECBTC#582 from Asherda/release-v1.2.3-6

45ec7a2

Release v1.2.3-6

Conversation

Alrighttt commented Mar 31, 2023

Uh oh!

dimxy commented Mar 31, 2023

Uh oh!

DeckerSU Mar 31, 2023

Choose a reason for hiding this comment

Uh oh!

Alrighttt Mar 31, 2023

Choose a reason for hiding this comment

Uh oh!

DeckerSU Mar 31, 2023

Choose a reason for hiding this comment

Uh oh!

DeckerSU Mar 31, 2023

Choose a reason for hiding this comment

Uh oh!

DeckerSU commented Apr 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dimxy commented Apr 1, 2023

Uh oh!

DeckerSU commented Apr 1, 2023

Uh oh!

dimxy commented Apr 5, 2023

Uh oh!

DeckerSU commented Apr 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dimxy commented Apr 6, 2023

Uh oh!

DeckerSU commented Apr 6, 2023

Uh oh!

dimxy commented May 18, 2023

Uh oh!

DeckerSU commented May 25, 2023

Uh oh!

dimxy commented May 25, 2023

Uh oh!

DeckerSU commented May 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dimxy May 30, 2023

Choose a reason for hiding this comment

Uh oh!

DeckerSU May 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimxy May 30, 2023

Choose a reason for hiding this comment

Uh oh!

dimxy commented May 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

dimxy commented May 30, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

DeckerSU commented Apr 1, 2023 •

edited

Loading

DeckerSU commented Apr 6, 2023 •

edited

Loading

DeckerSU commented May 25, 2023 •

edited

Loading

DeckerSU May 30, 2023 •

edited

Loading

dimxy commented May 30, 2023 •

edited

Loading