[PAN-2819] [PAN-2820] Mark Sweep #1638

RatanRSur · 2019-07-03T15:39:39Z

PR description

Brings in Mark Sweep Pruner and test associated with it.

Key differences from PoC:

PrunerTest.java
Waiting for a constant number of blocks before starting to mark to prevent wasted effort caused by reorgs

Fixed Issue(s)

https://pegasys1.atlassian.net/secure/RapidBoard.jspa?rapidView=28&modal=detail&selectedIssue=PAN-2820
https://pegasys1.atlassian.net/secure/RapidBoard.jspa?rapidView=28&modal=detail&selectedIssue=PAN-2819

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/worldstate/Pruner.java

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/worldstate/MarkSweepPruner.java

ajsutton

Looking good to me. I've noted a few things but nothing too major. Since this is based on code I wrote will leave actual approval to someone else.

...eum/blockcreation/src/main/java/tech/pegasys/pantheon/ethereum/blockcreation/BlockMiner.java

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/worldstate/Pruner.java

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/worldstate/MarkSweepPruner.java

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/worldstate/Pruner.java

ethereum/core/src/test/java/tech/pegasys/pantheon/ethereum/worldstate/PrunerTest.java

ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/DefaultSynchronizer.java

ethereum/trie/src/main/java/tech/pegasys/pantheon/ethereum/trie/StoredMerklePatriciaTrie.java

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/worldstate/Pruner.java

...vstore/src/test/java/tech/pegasys/pantheon/services/kvstore/AbstractKeyValueStorageTest.java

ethereum/core/src/test/java/tech/pegasys/pantheon/ethereum/worldstate/PrunerTest.java

...eum/core/src/main/java/tech/pegasys/pantheon/ethereum/worldstate/ExecutorServiceBuilder.java

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/worldstate/MarkSweepPruner.java

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/worldstate/Pruner.java

pantheon/src/main/java/tech/pegasys/pantheon/controller/PantheonControllerBuilder.java

…ll be flushed

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/worldstate/Pruner.java

shemnon

Just a couple nits, both optional. LGTM

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/worldstate/Pruner.java

shemnon · 2019-08-07T15:17:31Z

ethereum/core/src/test/java/tech/pegasys/pantheon/ethereum/worldstate/PrunerTest.java

+  @Test
+  public void shouldRejectInvalidArguments() {
+    final Blockchain mockchain = mock(Blockchain.class);
+    assertThatThrownBy(() -> new Pruner(markSweepPruner, mockchain, mockExecutorService, -1, -2))


nit: in a previous commit you covered multiple cases where only one arg was illegal (i.e. 0, -1 and what should have been -1, 0). I felt that test had more coverage.

Yes, meredith and I realized that the case that I thought was illegal is actually possible, so we relaxed the cases where the arguments were illegal.

RatanRSur commented Jul 3, 2019

View reviewed changes

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/worldstate/Pruner.java Outdated Show resolved Hide resolved

RatanRSur commented Jul 3, 2019

View reviewed changes

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/worldstate/MarkSweepPruner.java Outdated Show resolved Hide resolved

RatanRSur marked this pull request as ready for review July 5, 2019 19:30

RatanRSur requested a review from ajsutton July 10, 2019 14:31

RatanRSur changed the title ~~Mark Sweep~~ [PAN-2819] [PAN-2880] Mark Sweep Jul 10, 2019

RatanRSur changed the title ~~[PAN-2819] [PAN-2880] Mark Sweep~~ [PAN-2819] [PAN-2820] Mark Sweep Jul 10, 2019

ajsutton reviewed Jul 11, 2019

View reviewed changes

ajsutton reviewed Jul 16, 2019

View reviewed changes

ethereum/core/src/test/java/tech/pegasys/pantheon/ethereum/worldstate/PrunerTest.java Outdated Show resolved Hide resolved

ethereum/core/src/test/java/tech/pegasys/pantheon/ethereum/worldstate/PrunerTest.java Show resolved Hide resolved

ajsutton and others added 22 commits July 23, 2019 12:36

Mark Sweep Pruner - first pass

064409b

narrow Pruner dependency: ProtocolContext -> Blockchain

ee116fb

test scaffolding

0c6af0d

explainer comments and beginnings of block creation

7073fae

make Pruner.State public and add getter

a95ab99

sweeping log

023dee9

get test properly failing

aef1c1e

gate sweeping on marked block being on canonical chain

1af66ea

privitize state and remove getter

0bccea4

add transient fork outliving

adbc86a

add test check that markStorage is empty after sweeping

216907a

disable pruner for now

a69bf0f

set useColumns to false

4615be3

remove unused variable

f268c57

remove resolved TODOs

ef2499c

remove TODO that is now tracked by JIRA issue

26a5972

unmock BlockDataGenerator

d26b30c

remove outdated comment

c7224cd

remove mark/sweep irrelevant changes

6eaded2

make nodeAddedListenerID volatile

33a5b9d

don't exit on pruning failure

3641159

add mockito runner to pruner tests

a395a3d

RatanRSur added 12 commits July 23, 2019 12:36

fix exception string formatting

f4de1b4

spotless

fcb96b9

fix abort test after pruner state changing change

cd2fbec

build ExecutorService on demand

e57cd00

rename test

18de8a9

add pruner stopping to tests

71f2c61

give unloading responsibility to AllNodesVisitor

4367f7f

add override

4280ecc

remove more references to singleUseNodes

af061e9

remove unnecessary intellij refactoring

61645ef

Merge branch 'master' into mark-sweep

fcd6670

update to reflect WorldStatePreimage changes

105ff70

shemnon reviewed Aug 1, 2019

View reviewed changes

RatanRSur added 7 commits August 2, 2019 10:48

revert ExecutorServiceBuilder change and reduce shutdown timeout

2cffb78

make pruner cleanup pruning strategy on shutdown

8e95369

nest ifs to draw attention to side-effecting checks

896b5e3

add comment explaining interrupt handling

707ebca

remove unnecessary check to pending marks when sweeping because it wi…

3b7a020

…ll be flushed

relax argument requirements

bc032b1

extract block appending test util method

54ba517

mbaxter reviewed Aug 6, 2019

View reviewed changes

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/worldstate/Pruner.java Show resolved Hide resolved

RatanRSur added 4 commits August 6, 2019 15:30

fix IllegalArgumentException error message

aaa0191

revert cascading between states and fix tests to accomodate

5f3e57a

use "must" instead of "should" in exception message

1e01636

Merge branch 'master' into mark-sweep

320dd5e

shemnon approved these changes Aug 7, 2019

View reviewed changes

RatanRSur added 2 commits August 7, 2019 11:37

make executor Service final

245433f

Merge branch 'master' into mark-sweep

e4d119a

RatanRSur merged commit cb769a3 into PegaSysEng:master Aug 7, 2019

RatanRSur deleted the mark-sweep branch August 7, 2019 16:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PAN-2819] [PAN-2820] Mark Sweep #1638

[PAN-2819] [PAN-2820] Mark Sweep #1638

RatanRSur commented Jul 3, 2019 •

edited

Loading

ajsutton left a comment

shemnon left a comment

shemnon Aug 7, 2019

RatanRSur Aug 7, 2019

[PAN-2819] [PAN-2820] Mark Sweep #1638

[PAN-2819] [PAN-2820] Mark Sweep #1638

Conversation

RatanRSur commented Jul 3, 2019 • edited Loading

PR description

Fixed Issue(s)

ajsutton left a comment

Choose a reason for hiding this comment

shemnon left a comment

Choose a reason for hiding this comment

shemnon Aug 7, 2019

Choose a reason for hiding this comment

RatanRSur Aug 7, 2019

Choose a reason for hiding this comment

RatanRSur commented Jul 3, 2019 •

edited

Loading