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.

Sign up for GitHub

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

Jump to bottom

GC major update #2084

Merged
merged 3 commits into from
Mar 31, 2020
Merged

GC major update #2084

merged 3 commits into from
Mar 31, 2020

Conversation

Kouprin
Copy link
Member

@Kouprin Kouprin commented Feb 4, 2020 •
edited by ilblackdragon
Loading

Fixes #1648 and #1023

@Kouprin Kouprin requested a review from mikhailOK February 4, 2020 22:28
@Kouprin Kouprin requested a review from mikhailOK February 4, 2020 22:46
@codecov
Copy link

codecov bot commented Feb 4, 2020 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@050396a). Click here to learn what that means.
The diff coverage is 87.5%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2084   +/-   ##
=========================================
  Coverage          ?   86.84%           
=========================================
  Files             ?      180           
  Lines             ?    34362           
  Branches          ?        0           
=========================================
  Hits              ?    29841           
  Misses            ?     4521           
  Partials          ?        0
Impacted Files Coverage Δ
core/store/src/trie/state_parts.rs 96.63% <ø> (ø)
core/store/src/trie/trie_tests.rs 96.1% <ø> (ø)
core/store/src/db.rs 79.25% <ø> (ø)
chain/chain/src/types.rs 98.26% <ø> (ø)
core/store/src/lib.rs 89.47% <ø> (ø)
chain/chain/src/chain.rs 89.87% <100%> (ø)
chain/chain/src/test_utils.rs 85.35% <100%> (ø)
near/src/runtime.rs 70.48% <100%> (ø)
core/primitives/src/types.rs 97.67% <100%> (ø)
chain/client/src/client.rs 93.43% <100%> (ø)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 050396a...453a2c5. Read the comment docs.

@Kouprin Kouprin force-pushed the revert_insertions_into branch 3 times, most recently from 29dc1b5 to dbbb295 Compare February 5, 2020 02:04
@Kouprin Kouprin changed the title revert_insertions_into Trie cleaning Feb 5, 2020
mikhailOK
mikhailOK reviewed Feb 5, 2020
View reviewed changes
core/store/src/trie/mod.rs Outdated Show resolved Hide resolved
core/store/src/trie/mod.rs Outdated Show resolved Hide resolved
chain/chain/src/store.rs Outdated Show resolved Hide resolved
mikhailOK
mikhailOK reviewed Feb 5, 2020
View reviewed changes
chain/chain/src/store.rs Outdated Show resolved Hide resolved
@Kouprin Kouprin mentioned this pull request Feb 6, 2020
Closed
@weekly-digest weekly-digest bot mentioned this pull request Feb 7, 2020
Closed
@Kouprin Kouprin force-pushed the revert_insertions_into branch from dbbb295 to e6f2016 Compare February 18, 2020 14:21
@weekly-digest weekly-digest bot mentioned this pull request Feb 21, 2020
Closed
@ailisp ailisp changed the base branch from staging to master February 25, 2020 01:06
@weekly-digest weekly-digest bot mentioned this pull request Feb 28, 2020
Closed
@Kouprin Kouprin force-pushed the revert_insertions_into branch 8 times, most recently from 2cb9aa5 to cde720b Compare March 4, 2020 14:14
@Kouprin Kouprin force-pushed the revert_insertions_into branch from cde720b to 9962928 Compare March 10, 2020 10:13
@gitpod-io
Copy link

gitpod-io bot commented Mar 10, 2020 •
edited
Loading

@Kouprin Kouprin changed the title Trie cleaning GC major update Mar 10, 2020
@Kouprin Kouprin force-pushed the revert_insertions_into branch 2 times, most recently from 3f8aabd to e0679de Compare March 10, 2020 14:38
mikhailOK
mikhailOK suggested changes Mar 10, 2020
View reviewed changes
chain/chain/src/chain.rs Outdated Show resolved Hide resolved
chain/chain/src/store.rs Outdated Show resolved Hide resolved
chain/chain/src/store.rs Outdated Show resolved Hide resolved
@Kouprin
Copy link
Member Author

Kouprin commented Mar 23, 2020

#2270 merged successfully.
Reference map checking is added to GC testing module gc_fork_common.

@Kouprin Kouprin mentioned this pull request Mar 24, 2020
Merged
@Kouprin Kouprin force-pushed the revert_insertions_into branch 11 times, most recently from 1b7c04e to dbc58d0 Compare March 25, 2020 17:49
mikhailOK
mikhailOK reviewed Mar 26, 2020
View reviewed changes
chain/chain/src/store.rs Outdated Show resolved Hide resolved
chain/chain/src/store.rs Outdated Show resolved Hide resolved
core/store/src/trie/mod.rs Show resolved Hide resolved
chain/chain/src/store.rs Outdated Show resolved Hide resolved
chain/chain/src/store.rs Outdated Show resolved Hide resolved
chain/chain/src/store.rs Outdated Show resolved Hide resolved
core/store/src/test_utils.rs Outdated Show resolved Hide resolved
@Kouprin Kouprin force-pushed the revert_insertions_into branch from dbc58d0 to 4af83ab Compare March 26, 2020 06:49
frol
frol reviewed Mar 26, 2020
View reviewed changes
chain/chain/src/store.rs Outdated Show resolved Hide resolved
chain/chain/src/store.rs Outdated Show resolved Hide resolved
chain/chain/src/store.rs Outdated Show resolved Hide resolved
chain/chain/src/chain.rs Outdated Show resolved Hide resolved
chain/chain/src/chain.rs Outdated
}
chain_store_update.clear_block_data(trie.clone(), block_hash, false)?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we avoid having bools in "public" APIs? It is hard to reason about the code.

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't look like public API.

chain/chain/src/chain.rs Outdated Show resolved Hide resolved
chain/chain/src/store.rs Outdated Show resolved Hide resolved
@Kouprin Kouprin force-pushed the revert_insertions_into branch 2 times, most recently from 1191197 to 8d1096f Compare March 30, 2020 21:37
@Kouprin Kouprin requested a review from mikhailOK March 30, 2020 21:49
@Kouprin
Copy link
Member Author

Kouprin commented Mar 30, 2020

TEST PLAN

  1. Sanity store tests related to GC: test_clear_old_data and test_clear_old_data_fixed_height
  2. Lots of tests based on gc_fork_common() which creates forks, GCs them and validate the result.
    2a. Small tests added to CI.
    2b. Expensive tests added to Nightly.
  3. Adversarial and other python tests that checks refcount map validity.
  4. Sanity GC python tests that are running on Nightshade runtime (tests/sanity/garbage_collection*.py).

@Kouprin Kouprin force-pushed the revert_insertions_into branch from 8d1096f to a7bec30 Compare March 30, 2020 22:09
@bowenwang1996 bowenwang1996 merged commit 5254f85 into master Mar 31, 2020
@bowenwang1996 bowenwang1996 deleted the revert_insertions_into branch March 31, 2020 00:40
@Kouprin Kouprin mentioned this pull request Mar 31, 2020
Closed
This was referenced Apr 1, 2020
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bowenwang1996 bowenwang1996 bowenwang1996 approved these changes

@frol frol frol approved these changes

@mikhailOK mikhailOK mikhailOK approved these changes

@evgenykuzyakov evgenykuzyakov Awaiting requested review from evgenykuzyakov

@MaksymZavershynskyi MaksymZavershynskyi Awaiting requested review from MaksymZavershynskyi

@SkidanovAlex SkidanovAlex Awaiting requested review from SkidanovAlex

Assignees

@SkidanovAlex SkidanovAlex

@mikhailOK mikhailOK

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Delete old states
5 participants
@Kouprin @bowenwang1996 @frol @SkidanovAlex @mikhailOK