Add support for multiple staging areas #1633

svarogg · 2021-03-25T12:54:33Z

This PR enables to address #1516

…s-lots-of-memory

…hrough store

codecov · 2021-03-25T13:06:19Z

Codecov Report

Merging #1633 (71afd1d) into v0.10.0-dev (c12e180) will increase coverage by 0.23%.
The diff coverage is 82.32%.

@@               Coverage Diff               @@
##           v0.10.0-dev    #1633      +/-   ##
===============================================
+ Coverage        59.95%   60.18%   +0.23%     
===============================================
  Files              516      532      +16     
  Lines            20469    20673     +204     
===============================================
+ Hits             12273    12443     +170     
- Misses            6239     6267      +28     
- Partials          1957     1963       +6

Impacted Files	Coverage Δ
...tures/consensusstatestore/consensus_state_store.go	`66.66% <0.00%> (ø)`
...structures/consensusstatestore/virtual_utxo_set.go	`44.11% <ø> (+2.45%)`	⬆️
domain/consensus/factory.go	`91.17% <ø> (ø)`
...ain/consensus/processes/coinbasemanager/payload.go	`68.96% <ø> (ø)`
domain/consensus/test_consensus.go	`65.59% <ø> (-4.22%)`	⬇️
domain/consensus/test_consensus_render_to_dot.go	`0.00% <0.00%> (ø)`
...ersselectedtipstore/headers_selected_tips_store.go	`46.66% <46.66%> (ø)`
...res/blockheaderstore/block_header_staging_shard.go	`51.61% <51.61%> (ø)`
...tructures/utxodiffstore/utxo_diff_staging_shard.go	`54.54% <54.54%> (ø)`
...uctures/daablocksstore/daa_blocks_staging_shard.go	`54.83% <54.83%> (ø)`
... and 108 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 c12e180...71afd1d. Read the comment docs.

stasatdaglabs · 2021-03-28T07:19:10Z

domain/consensus/model/staging_area.go

+
+	for _, shard := range sa.shards {
+		if shard == nil {
+			continue


Why would a shard ever be nil? Since this may hide bugs, I'd prefer it if you returned an error here.

See response to next comment, it should explain this one as well.

stasatdaglabs · 2021-03-28T07:44:07Z

domain/consensus/model/staging_area.go

+
+// GetOrCreateShard attempts to retrieve a shard with the given name.
+// If it does not exist - a new shard is created using `createFunc`.
+func (sa *StagingArea) GetOrCreateShard(shardID StagingShardID, createFunc func() StagingShard) StagingShard {


Since GetOrCreateShard is only ever called from within a store, please consider passing a pointer to a store here instead of a shardID. This would:
a. Remove the need to maintain an IDs list
b. Disallow programmers from accessing shards they shouldn't have access to (e.g. in one store, another store's shard)
Of course, make sure that this does not harm performance.

Please note sa.shards is an array.
This was done deliberately, because map access is much slower (I've tested this with integers, I'd expect the same to happen with pointers, because those are basically integers as well).

This, as you mentioned, has some worse properties, but using a map adds around 20~30 overhead for kaspad normal operation.

This also explains why in Commit a shard might be nil - we are going over all the array every time we Commit. Some slots in the array could be empty.

I've added some comments explaining this, please LMK if there's anywhere else you think should be augmented, or if you have some ideas to how we can improve on this.

stasatdaglabs · 2021-03-28T07:47:24Z

domain/consensus/datastructures/acceptancedatastore/acceptance_data_staging_shard.go

+}
+
+func (ads *acceptanceDataStore) stagingShard(stagingArea *model.StagingArea) *acceptanceDataStagingShard {
+	return stagingArea.GetOrCreateShard(model.StagingShardIDAcceptanceData, func() model.StagingShard {


Please extract the create function to a separate method.
I'm pretty sure that currently, every time stagingShard and similar functions are called, Go creates a new function along with a closure over the current scope. I imagine that to be quite costly.
Please do run benchmarks to make sure I'm not wrong though.

BenchmarkStagingShardClosure-8 467047196 2.787 ns/op BenchmarkStagingShardNoClosure-8 405023798 2.945 ns/op

They are virtually the same (actually NoClosure is a bit slower, I think I know why, but is miniscule difference anyway).
So seems like there's no point.

For reference, here's implemention of NoClosure:

func (ads *acceptanceDataStore) createStagingShard() model.StagingShard { return &acceptanceDataStagingShard{ store: ads, toAdd: make(map[externalapi.DomainHash]externalapi.AcceptanceData), toDelete: make(map[externalapi.DomainHash]struct{}), } } func (ads *acceptanceDataStore) stagingShardNoClosure(stagingArea *model.StagingArea) *acceptanceDataStagingShard { return stagingArea.GetOrCreateShard(model.StagingShardIDAcceptanceData, ads.createStagingShard).(*acceptanceDataStagingShard) }

stasatdaglabs · 2021-03-28T08:01:19Z

domain/consensus/datastructures/acceptancedatastore/acceptance_data_staging_shard.go

+	}).(*acceptanceDataStagingShard)
+}
+
+func (adss acceptanceDataStagingShard) Commit(dbTx model.DBTransaction) error {


Please go over all the shard types and make sure that the method receivers are pointers.

stasatdaglabs · 2021-03-28T08:26:23Z

domain/consensus/datastructures/blockstore/block_store.go

@@ -11,22 +11,18 @@ import (
 )

 var bucket = database.MakeBucket([]byte("blocks"))
-var countKey = database.MakeBucket(nil).Key([]byte("blocks-count"))
+var countKey = database.MakeBucket(nil).Key([]byte("blocks-countCached"))


Did you mean to do this?

Nope :/
Good catch. This would have turned my change to db-breaking.

stasatdaglabs · 2021-03-28T08:31:33Z

domain/consensus/datastructures/consensusstatestore/tips.go

@@ -72,3 +56,21 @@ func (css *consensusStateStore) deserializeTips(tipsBytes []byte) ([]*externalap

 	return serialization.DBTipsToTips(dbTips)
 }
+
+func (csss *consensusStateStagingShard) commitTips(dbTx model.DBTransaction) error {


Please consider moving this to consensus_state_staging_shard.go.

Hmmm... this and commitVirtualUTXODiff were deliberately left in their respective files, since consensus-state-store is split to file according to themes, not types.

Thus I felt it makes more sense to leave them here, if you disagree lmk, I don't mind moving it to consensus_state_staging_shard.go too much

stasatdaglabs · 2021-03-28T08:31:43Z

domain/consensus/datastructures/consensusstatestore/utxo.go

 }

-func (css *consensusStateStore) commitVirtualUTXODiff(dbTx model.DBTransaction) error {
-	hadStartedImportingPruningPointUTXOSet, err := css.HadStartedImportingPruningPointUTXOSet(dbTx)
+func (csss *consensusStateStagingShard) commitVirtualUTXODiff(dbTx model.DBTransaction) error {


Please consider moving this to consensus_state_staging_shard.go.

stasatdaglabs · 2021-03-28T08:35:53Z

domain/consensus/datastructures/headersselectedtipstore/headers_selected_tip_staging_shard.go

+
+func (hstss headersSelectedTipStagingShard) Commit(dbTx model.DBTransaction) error {
+	if hstss.newSelectedTip == nil {
+		return nil


Now that not all stores are necessarily committed, please check whether this should return an error instead.

The moment someone does a read from a store it's staging shard is created.
Therefore, a staging shard might exist (and then it's Commit function will be called) while nothing needs to be commited.

stasatdaglabs · 2021-03-28T08:37:07Z

domain/consensus/model/staging_area.go

+// Since Commit happens in a DatabaseTransaction, a StagingArea is atomic.
+type StagingArea struct {
+	shards     [StagingShardIDLen]StagingShard
+	isCommited bool


Typo: isCommited -> isCommitted

…516-staging-area

svarogg added 30 commits March 17, 2021 18:03

Add StagingArea struct

42ff6f1

Implemented staging areas in blockStore

fe32c69

Merge remote-tracking branch 'origin/v0.10.0-dev' into 1516-big-reorg…

1923691

…s-lots-of-memory

Move blockStagingShard to separate folder

5f75a7d

Apply staging shard to acceptanceDataStore

861e5b6

Update blockHeaderStore with StagingArea

3fc8ebf

Add StagingArea to BlockRelationStore

ef1ca8c

Add StagingArea to blockStatusStore

fa76eca

Add StagingArea to consensusStateStore

2a559dc

Add StagingArea to daaBlocksStore

021ec66

Add StagingArea to finalityStore

579e765

Add StagingArea to ghostdagDataStore

dfbf4d8

Add StagingArea to headersSelectedChainStore and headersSelectedTipStore

ef15eb5

Add StagingArea to multisetStore

e276a4d

Add StagingArea to pruningStore

e6e9a59

Add StagingArea to reachabilityDataStore

8d8e604

Add StagingArea to utxoDiffStore

b8270a3

Fix forgotten compilation error

daeffe6

Update reachability manager and some more things with StagingArea

c55cda3

Add StagingArea to dagTopologyManager, and some more

30b5926

Add StagingArea to GHOSTDAGManager, and some more

75320fb

Add StagingArea to difficultyManager, and some more

dea65ac

Add StagingArea to dagTraversalManager, and some more

b746424

Add StagingArea to headerTipsManager, and some more

e67ceb3

Add StagingArea to constnsusStateManager, pastMedianTimeManager

fd1821d

Add StagingArea to transactionValidator

efdd029

Add StagingArea to finalityManager

25ec425

Add StagingArea to mergeDepthManager

c92ae20

Add StagingArea to pruningManager

bd78989

Add StagingArea to rest of ValidateAndInsertBlock

d4a0ad2

svarogg added 17 commits March 24, 2021 09:43

Mark stagingShard.isCommited = true once commited

b2fb81f

Move isStaged to stagingShard, so that it's available without going t…

ebc74bc

…hrough store

Make blockHeaderStore count be avilable from stagingShard

624dd62

Fix remaining forgotten stagingArea passing

f3ccf71

commitAllChanges should call dbTx.Commit in the end

62f96b9

Fix all tests tests in blockValidator

a6dbb4d

Fix all tests in consensusStateManager and some more

a852ba1

Fix all tests in pruningManager

1b3013a

Add many missing stagingAreas in tests

f443ccb

Fix many tests

863ff66

Fix most of all other tests

bde165b

Fix ghostdag_test.go

7f1c9e6

Add comment to StagingArea

9dbd8cb

Merge branch 'v0.10.0-dev' into 1516-staging-area

57590c4

Merge remote-tracking branch 'origin/v0.10.0-dev' into 1516-staging-area

d22bd85

Make list of StagingShards an array

b1ff436

Add comment to StagingShardID

5d52e69

svarogg mentioned this pull request Mar 25, 2021

Do resolveSingleBlockStatus in a separate StagingArea to prevent huge db transaction #1634

Merged

stasatdaglabs requested changes Mar 28, 2021

View reviewed changes

svarogg added 6 commits March 29, 2021 09:43

Merge branch 'v0.10.0-dev' into 1516-staging-area

1bac6c8

Make sure all staging shards are pointer-receiver

ba7fc70

Undo bucket rename in block_store

bd80d39

Typo: isCommited -> isCommitted

3302abb

Add comment explaining why stagingArea.shards is an array

1fd7cf3

Merge branch '1516-staging-area' of github.com:kaspanet/kaspad into 1…

71afd1d

…516-staging-area

stasatdaglabs approved these changes Mar 29, 2021

View reviewed changes

svarogg merged commit f145140 into v0.10.0-dev Mar 29, 2021

svarogg deleted the 1516-staging-area branch March 29, 2021 07:34

stasatdaglabs mentioned this pull request Apr 20, 2021

Big reorgs take a lot of memory #1516

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for multiple staging areas #1633

Add support for multiple staging areas #1633

svarogg commented Mar 25, 2021 •

edited

codecov bot commented Mar 25, 2021 •

edited

stasatdaglabs Mar 28, 2021

svarogg Mar 29, 2021

stasatdaglabs Mar 28, 2021

svarogg Mar 29, 2021

stasatdaglabs Mar 28, 2021

svarogg Mar 29, 2021

stasatdaglabs Mar 28, 2021

stasatdaglabs Mar 28, 2021

svarogg Mar 29, 2021

stasatdaglabs Mar 28, 2021

svarogg Mar 29, 2021

stasatdaglabs Mar 28, 2021

stasatdaglabs Mar 28, 2021

svarogg Mar 29, 2021

stasatdaglabs Mar 28, 2021

Add support for multiple staging areas #1633

Add support for multiple staging areas #1633

Conversation

svarogg commented Mar 25, 2021 • edited

codecov bot commented Mar 25, 2021 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svarogg commented Mar 25, 2021 •

edited

codecov bot commented Mar 25, 2021 •

edited