search

near / nearcore

Reference client for NEAR Protocol
https://near.org
GNU General Public License v3.0
2.3k stars 601 forks source link

Unload obsolete memtries post block processing #11657

Closed staffik closed 1 week ago

staffik commented 2 weeks ago

Currently we remove obsolete memtries only when we start state sync for a new shard: https://github.com/near/nearcore/blob/master/chain/client/src/client.rs#L2507-L2519 However, it does not unload obsolete memtries in all cases. For example, if we tracked shards [0, 1, 2], and then we track shards [0, 1] only, shard 2 won't be unloaded, because we did not start a new state sync. Although it is not critical, trie for shard 2 could occupy memory forever.

This PR calls the routine to unload obsolete memtries as a block post-processing step too.

codecov[bot] commented 2 weeks ago

Codecov Report

Attention: Patch coverage is 91.30435% with 2 lines in your changes missing coverage. Please review.

Project coverage is 71.61%. Comparing base (74ac5fe) to head (f58c2a1). Report is 3 commits behind head on master.

Files Patch % Lines
chain/chain/src/chain.rs 91.30% 0 Missing and 2 partials :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #11657 +/- ## ========================================== - Coverage 71.65% 71.61% -0.05% ========================================== Files 787 787 Lines 161063 161104 +41 Branches 161063 161104 +41 ========================================== - Hits 115413 115373 -40 - Misses 40597 40684 +87 + Partials 5053 5047 -6 ``` | [Flag](https://app.codecov.io/gh/near/nearcore/pull/11657/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | Coverage Δ | | |---|---|---| | [backward-compatibility](https://app.codecov.io/gh/near/nearcore/pull/11657/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `0.23% <0.00%> (-0.01%)` | :arrow_down: | | [db-migration](https://app.codecov.io/gh/near/nearcore/pull/11657/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `0.23% <0.00%> (-0.01%)` | :arrow_down: | | [genesis-check](https://app.codecov.io/gh/near/nearcore/pull/11657/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `1.36% <0.00%> (-0.01%)` | :arrow_down: | | [integration-tests](https://app.codecov.io/gh/near/nearcore/pull/11657/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `37.80% <91.30%> (+0.06%)` | :arrow_up: | | [linux](https://app.codecov.io/gh/near/nearcore/pull/11657/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `69.00% <91.30%> (-0.04%)` | :arrow_down: | | [linux-nightly](https://app.codecov.io/gh/near/nearcore/pull/11657/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `71.10% <91.30%> (-0.07%)` | :arrow_down: | | [macos](https://app.codecov.io/gh/near/nearcore/pull/11657/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `52.61% <91.30%> (+0.04%)` | :arrow_up: | | [pytests](https://app.codecov.io/gh/near/nearcore/pull/11657/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `1.59% <0.00%> (-0.01%)` | :arrow_down: | | [sanity-checks](https://app.codecov.io/gh/near/nearcore/pull/11657/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `1.39% <0.00%> (-0.01%)` | :arrow_down: | | [unittests](https://app.codecov.io/gh/near/nearcore/pull/11657/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `66.22% <91.30%> (-0.03%)` | :arrow_down: | | [upgradability](https://app.codecov.io/gh/near/nearcore/pull/11657/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `0.28% <0.00%> (-0.01%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Longarithm commented 1 week ago

That sounds right. However, still looks hacky that we are doing this check for every single block, while the shard "status" changes only once per epoch.

I have a feeling that we can call retain_mem_tries only if epoch_manager.is_next_block_epoch_start(parent_hash) is true, and remove retain_mem_tries from run_catchup because it doesn't fit there. Would you like to try it?

Also nit - if the function is called retain_mem_tries, the comment should also explain why retaining happens. The fact that it mentions GC (the opposite event) is confusing.