spacemeshos / go-spacemesh

Go Implementation of the Spacemesh protocol full node. 💾⏰💪
https://spacemesh.io
MIT License
748 stars 211 forks source link

[Merged by Bors] - fix slow lookup of previous ATX for (ID, epoch) #6314

Closed poszu closed 3 weeks ago

poszu commented 3 weeks ago

Motivation

Speed up atxs.PrevIdByNodeId

Description

The query used in atxs.PrevIdByNodeId became very slow in 1.7 because it joins atxs and posts table to filter by the epoch from the atxs table.

Before:

Benchmark_PrevIdByNodeID/1.7-20                 1960        533631 ns/op         714 B/op         19 allocs/op
Benchmark_PrevIdByNodeID/1.6-20                74451         17354 ns/op         714 B/op         19 allocs/op

I added publish_epoch column to the posts table to avoid joining the two tables.

After the change, the performance is equal to the original code:

Benchmark_PrevIdByNodeID/1.6-20                56253         18199 ns/op         713 B/op         19 allocs/op
Benchmark_PrevIdByNodeID/1.7-20                61796         18966 ns/op         713 B/op         19 allocs/op

The benchmark

The benchmark code is not included in the PR because there isn't the "old" implementation to compare with. Below is its code:

func Benchmark_PrevIdByNodeID(b *testing.B) {
    db := statesql.InMemoryTest(b)

    var signers []*signing.EdSigner

    for i := 0; i < 1000; i++ {
        sig, err := signing.NewEdSigner()
        require.NoError(b, err)
        signers = append(signers, sig)
    }

    for epoch := range types.EpochID(30) {
        var prev types.ATXID
        for _, signer := range signers {
            atx := newAtx(b, signer, withPublishEpoch(epoch))
            require.NoError(b, atxs.Add(db, atx, types.AtxBlob{}))
            require.NoError(b, atxs.SetPost(db, atx.ID(), prev, 0, signer.NodeID(), 4, atx.PublishEpoch))
            prev = atx.ID()
        }
    }

    b.Run("1.6", func(b *testing.B) {
        for i := 0; i < b.N; i++ {
            _, err := atxs.PrevIDByNodeID_OLD(db, signers[i%len(signers)].NodeID(), 2)
            require.NoError(b, err)
        }
    })
    b.Run("1.7", func(b *testing.B) {
        for i := 0; i < b.N; i++ {
            _, err := atxs.PrevIDByNodeID(db, signers[i%len(signers)].NodeID(), 20)
            require.NoError(b, err)
        }
    })
}

Test Plan

TODO

codecov[bot] commented 3 weeks ago

Codecov Report

Attention: Patch coverage is 81.81818% with 4 lines in your changes missing coverage. Please review.

Project coverage is 81.8%. Comparing base (f5f96b9) to head (1075dd0). Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
sql/statesql/migrations/state_0021_migration.go 75.0% 1 Missing and 2 partials :warning:
sql/atxs/atxs.go 87.5% 0 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #6314 +/- ## ========================================= - Coverage 81.8% 81.8% -0.1% ========================================= Files 312 312 Lines 34603 34616 +13 ========================================= + Hits 28310 28316 +6 - Misses 4458 4469 +11 + Partials 1835 1831 -4 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

fasmat commented 3 weeks ago

This changes an existing migration. Does that mean all nodes running 1.7.0-alphaX cannot be upgraded to the next 1.7.x release because their DB migrations might fail to match the expected schema?

EDIT: nvm, I misunderstood what impact the modification would have - ignore this comment 🙂

poszu commented 3 weeks ago

Bors merge

spacemesh-bors[bot] commented 3 weeks ago

Pull request successfully merged into develop.

Build succeeded: