Skip to content

fix: improve performance of ApplyPowerTableDiffs in ImportSnapshotToDatastore #1047

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

hanabi1224
Copy link
Contributor

@hanabi1224 hanabi1224 commented Aug 1, 2025

The sort operation in ApplyPowerTableDiffs turns out to be expensive.
This PR introduces another func ApplyPowerTableDiffsToMap(powerTableMap map[gpbft.ActorID]gpbft.PowerEntry, diffs ...PowerTableDiff) (map[gpbft.ActorID]gpbft.PowerEntry, error) to avoid sorting.

It also adds more validations against first instance, latest instance and powertable cid.

Perf stats of importing a mainnet F3 snapshot on my laptop show ~99% perf gain

# PR
0.884s
0.773s
0.864s

# main
86.547s
88.422s
86.998s

@github-project-automation github-project-automation bot moved this to Todo in F3 Aug 1, 2025
Copy link

codecov bot commented Aug 1, 2025

Codecov Report

❌ Patch coverage is 53.96825% with 29 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.38%. Comparing base (9288fba) to head (c04cb35).

Files with missing lines Patch % Lines
certstore/snapshot.go 30.95% 18 Missing and 11 partials ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1047      +/-   ##
==========================================
- Coverage   64.77%   64.38%   -0.40%     
==========================================
  Files          81       81              
  Lines        9896     9942      +46     
==========================================
- Hits         6410     6401       -9     
- Misses       2961     3005      +44     
- Partials      525      536      +11     
Files with missing lines Coverage Δ
certs/certs.go 94.28% <100.00%> (+0.34%) ⬆️
certstore/snapshot.go 41.61% <30.95%> (-6.22%) ⬇️

... and 5 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@hanabi1224 hanabi1224 marked this pull request as ready for review August 1, 2025 16:28
@hanabi1224 hanabi1224 force-pushed the speed-up-apply-power-table-diffs branch from 5fbd9b1 to af0db51 Compare August 4, 2025 09:30
@rjan90 rjan90 moved this from Todo to In review in F3 Aug 4, 2025
@rjan90 rjan90 requested a review from Kubuxu August 4, 2025 14:14
@BigLep BigLep requested a review from Copilot August 12, 2025 03:02
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR optimizes the performance of applying power table diffs during F3 snapshot import by introducing a new map-based approach that avoids expensive sorting operations on each iteration. The optimization shows a ~99% performance improvement when importing mainnet F3 snapshots.

  • Introduced ApplyPowerTableDiffsToMap function to work with power table maps instead of sorted arrays
  • Added comprehensive validation for instance numbers, latest instance bounds, and power table CID verification
  • Refactored importSnapshotToDatastoreWithTestingPowerTableFrequency to use the new map-based approach and include additional safety checks

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
certstore/snapshot.go Refactored snapshot import logic to use map-based power table operations and added validation checks
certs/certs.go Added new map-based power table diff functions and helper functions for array-map conversion

return err
}

return cs.writeInstanceNumber(ctx, certStoreLatestKey, header.LatestInstance)
Copy link
Preview

Copilot AI Aug 12, 2025

Choose a reason for hiding this comment

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

The function writes header.LatestInstance but should write the actual latest processed instance. If the loop breaks early due to EOF, this could write an incorrect latest instance number.

Suggested change
return cs.writeInstanceNumber(ctx, certStoreLatestKey, header.LatestInstance)
return cs.writeInstanceNumber(ctx, certStoreLatestKey, latestProcessedInstance)

Copilot uses AI. Check for mistakes.

Copy link
Contributor

Choose a reason for hiding this comment

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

This also seems valid.
Alternative is erroring out if header.Latest is different from latestCert.GPBFTInstance

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed by returning err when latestCert.GPBFTInstance != header.LatestInstance

if err := cs.putPowerTable(ctx, cert.GPBFTInstance+1, pt); err != nil {
return err
}
}
}

pt := certs.PowerTableMapToArray(ptm)
Copy link
Preview

Copilot AI Aug 12, 2025

Choose a reason for hiding this comment

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

Potential nil pointer dereference if no certificates are processed (empty snapshot). latestCert will be nil if the loop never executes due to immediate EOF.

Suggested change
pt := certs.PowerTableMapToArray(ptm)
pt := certs.PowerTableMapToArray(ptm)
if latestCert == nil {
return ErrUnknownLatestCertificate
}

Copilot uses AI. Check for mistakes.

Copy link
Contributor

@Kubuxu Kubuxu Aug 12, 2025

Choose a reason for hiding this comment

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

This seems to be a valid issue. The suggested resolution could be better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed by adding nil check

@Kubuxu
Copy link
Contributor

Kubuxu commented Aug 12, 2025

SGTM apart from the two issues highlighted by Copilot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In review
Development

Successfully merging this pull request may close these issues.

2 participants