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.

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

Make moving the pruning point faster #1660

Merged
merged 7 commits into from
Apr 11, 2021
Merged

Make moving the pruning point faster #1660

merged 7 commits into from
Apr 11, 2021

Conversation

elichai
Copy link
Member

@elichai elichai commented Apr 5, 2021

Related to #1629, this makes moving the pruning point much faster (2-4 times faster)
I kept the commitment validation for now, but if everything works fine in ~2 weeks, then I think we can remove it to improve performance.

This should also help us do this in a separate thread, as it no longer has anything to do with the virtual.

@elichai elichai added this to In progress in Mainnet via automation Apr 5, 2021
@codecov
Copy link

codecov bot commented Apr 5, 2021

Codecov Report

Merging #1660 (896941d) into v0.10.0-dev (9bb8123) will decrease coverage by 0.07%.
The diff coverage is 58.26%.

❗ Current head 896941d differs from pull request most recent head b4f991e. Consider uploading reports for the commit b4f991e to get more accurate results
Impacted file tree graph

@@               Coverage Diff               @@
##           v0.10.0-dev    #1660      +/-   ##
===============================================
- Coverage        59.54%   59.47%   -0.08%     
===============================================
  Files              548      548              
  Lines            21695    21789      +94     
===============================================
+ Hits             12919    12958      +39     
- Misses            6726     6769      +43     
- Partials          2050     2062      +12     
Impacted Files Coverage Δ
...ensus/datastructures/pruningstore/pruning_store.go 52.45% <48.57%> (-4.97%) ⬇️
...nsensus/processes/pruningmanager/pruningmanager.go 59.11% <60.52%> (-0.28%) ⬇️
...tastructures/pruningstore/pruning_staging_shard.go 60.52% <64.28%> (-1.55%) ⬇️
domain/consensus/factory.go 91.25% <100.00%> (ø)
...processes/blockprocessor/validateandinsertblock.go 70.90% <100.00%> (ø)
...nsensus/datastructures/consensusstatestore/tips.go 33.33% <0.00%> (-6.07%) ⬇️
...in/consensus/utils/utxo/utxo_iterator_with_diff.go 61.66% <0.00%> (-5.00%) ⬇️
domain/consensus/utils/utxo/mutable_utxo_diff.go 62.12% <0.00%> (-4.55%) ⬇️
domain/consensus/utils/utxo/immutable_utxo_diff.go 46.87% <0.00%> (-3.13%) ⬇️
...nsensus/datastructures/consensusstatestore/utxo.go 44.60% <0.00%> (-1.44%) ⬇️
... and 3 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 9bb8123...b4f991e. Read the comment docs.

@@ -9,6 +9,7 @@ type pruningStagingShard struct {
store *pruningStore

newPruningPoint *externalapi.DomainHash
oldPruningPoint *externalapi.DomainHash
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please rename newPruningPoint to currentPruningPoint and oldPruningPoint to previousPruningPoint here and everywhere else.

// Insert all the new UTXOs into the database
for ok := utxoSetIterator.First(); ok; ok = utxoSetIterator.Next() {
outpoint, entry, err := utxoSetIterator.Get()
// Delete all the old UTXOs from the database
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove this comment. It's a duplicate of the one in line 90.

toAddIterator := diff.ToAdd().Iterator()
defer toAddIterator.Close()
for ok := toAddIterator.First(); ok; ok = toAddIterator.Next() {
toRemoveOutpoint, entry, err := toAddIterator.Get()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please rename toRemoveOutpoint to toAddOutpoint.

@@ -535,28 +615,43 @@ func (pm *pruningManager) UpdatePruningPointUTXOSetIfRequired() error {
return nil
}

func (pm *pruningManager) updatePruningPointUTXOSet(stagingArea *model.StagingArea) error {
func (pm *pruningManager) updatePruningPointUTXOSet() error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It appears that now you're also deleting data within this function. Please update its name accordingly.

Copy link
Member Author

@elichai elichai Apr 8, 2021

Choose a reason for hiding this comment

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

This function always removed and added data (removed the old pruning point UTXO set and added the new pruning UTXO set)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I meant deleting block (and other) data. As in, the actual pruning.

err := pm.validateUTXOSetFitsCommitment(stagingArea, pruningPointHash)
if err != nil {
oldPruningPoint, err := pm.pruningStore.PruningPoint(pm.databaseContext, stagingArea)
if err != nil && !errors.Is(err, database.ErrNotFound) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd prefer to handle the genesis block explicitly instead of checking whether the pruning point happens to exist in the database.

return nil, err
}

// do the quick restore.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you mean to delete this?

@@ -468,6 +466,88 @@ func (pm *pruningManager) validateUTXOSetFitsCommitment(stagingArea *model.Stagi
return nil
}

func (pm *pruningManager) calculateDiffBetweenOldAndNewPruningPoints(stagingArea *model.StagingArea, newPruningHash *externalapi.DomainHash) (externalapi.UTXODiff, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add a comment explaining how this function works.

oldPruningCurrentDiffChildBlueWork := oldPruningGhostDAG.BlueWork()

var oldDiffs []externalapi.UTXODiff
var newDiffs []externalapi.UTXODiff
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please rename the above to previousPruningPointUTXODiffs and currentPruningPointUTXODiffs.

}
}
oldDiff := utxo.NewMutableUTXODiff()
for i := len(oldDiffs) - 1; i >= 0; i-- {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please explain in a comment why these diffs need to be applied in reverse order.

@elichai elichai merged commit 3c3ad14 into v0.10.0-dev Apr 11, 2021
Mainnet automation moved this from In progress to Done Apr 11, 2021
@elichai elichai deleted the optimize-pruning branch April 11, 2021 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants