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

perf: avoid copy of period locks in epoch #7192

Merged
merged 1 commit into from
Dec 22, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 11 additions & 6 deletions x/incentives/keeper/distribute.go
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,7 @@ func (k Keeper) doDistributionSends(ctx sdk.Context, distrs *distributionInfo) e
// the distrInfo struct. It also updates the gauge for the distribution.
// locks is expected to be the correct set of lock recipients for this gauge.
func (k Keeper) distributeSyntheticInternal(
ctx sdk.Context, gauge types.Gauge, locks []lockuptypes.PeriodLock, distrInfo *distributionInfo,
ctx sdk.Context, gauge types.Gauge, locks []*lockuptypes.PeriodLock, distrInfo *distributionInfo,
) (sdk.Coins, error) {
qualifiedLocks := k.lk.GetLocksLongerThanDurationDenom(ctx, gauge.DistributeTo.Denom, gauge.DistributeTo.Duration)

Expand All @@ -398,12 +398,17 @@ func (k Keeper) distributeSyntheticInternal(
}
}

sortedAndTrimmedQualifiedLocks := make([]lockuptypes.PeriodLock, curIndex)
sortedAndTrimmedQualifiedLocks := make([]*lockuptypes.PeriodLock, curIndex)
// This is not an issue because we directly
// use v.index and &v.locks. However, we must be careful not to
// take the address of &v.
// nolint: exportloopref
for _, v := range qualifiedLocksMap {
v := v
if v.index < 0 {
continue
}
sortedAndTrimmedQualifiedLocks[v.index] = v.lock
sortedAndTrimmedQualifiedLocks[v.index] = &v.lock
}

return k.distributeInternal(ctx, gauge, sortedAndTrimmedQualifiedLocks, distrInfo)
Expand Down Expand Up @@ -556,7 +561,7 @@ func (k Keeper) syncVolumeSplitGroup(ctx sdk.Context, group types.Group) error {
//
// CONTRACT: gauge passed in as argument must be an active gauge.
func (k Keeper) distributeInternal(
ctx sdk.Context, gauge types.Gauge, locks []lockuptypes.PeriodLock, distrInfo *distributionInfo,
ctx sdk.Context, gauge types.Gauge, locks []*lockuptypes.PeriodLock, distrInfo *distributionInfo,
) (sdk.Coins, error) {
totalDistrCoins := sdk.NewCoins()

Expand Down Expand Up @@ -757,10 +762,10 @@ func (k Keeper) handleGroupPostDistribute(ctx sdk.Context, groupGauge types.Gaug
}

// getDistributeToBaseLocks takes a gauge along with cached period locks by denom and returns locks that must be distributed to
func (k Keeper) getDistributeToBaseLocks(ctx sdk.Context, gauge types.Gauge, cache map[string][]lockuptypes.PeriodLock) []lockuptypes.PeriodLock {
func (k Keeper) getDistributeToBaseLocks(ctx sdk.Context, gauge types.Gauge, cache map[string][]lockuptypes.PeriodLock) []*lockuptypes.PeriodLock {
// if gauge is empty, don't get the locks
if gauge.Coins.Empty() {
return []lockuptypes.PeriodLock{}
return []*lockuptypes.PeriodLock{}
}
// Confusingly, there is no way to get all synthetic lockups. Thus we use a separate method `distributeSyntheticInternal` to separately get lockSum for synthetic lockups.
// All gauges have a precondition of being ByDuration.
Expand Down
10 changes: 5 additions & 5 deletions x/incentives/keeper/iterator.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,11 +63,11 @@ func (k Keeper) FinishedGaugesIterator(ctx sdk.Context) sdk.Iterator {
}

// FilterLocksByMinDuration returns locks whose lock duration is greater than the provided minimum duration.
func FilterLocksByMinDuration(locks []lockuptypes.PeriodLock, minDuration time.Duration) []lockuptypes.PeriodLock {
filteredLocks := make([]lockuptypes.PeriodLock, 0, len(locks))
for _, lock := range locks {
if lock.Duration >= minDuration {
filteredLocks = append(filteredLocks, lock)
func FilterLocksByMinDuration(locks []lockuptypes.PeriodLock, minDuration time.Duration) []*lockuptypes.PeriodLock {
filteredLocks := make([]*lockuptypes.PeriodLock, 0, len(locks))
for i := range locks {
if locks[i].Duration >= minDuration {
filteredLocks = append(filteredLocks, &locks[i])
}
}
return filteredLocks
Expand Down
37 changes: 37 additions & 0 deletions x/incentives/keeper/iterator_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
package keeper_test

import (
"testing"

"github.com/stretchr/testify/require"

"github.com/osmosis-labs/osmosis/v21/x/incentives/keeper"
lockuptypes "github.com/osmosis-labs/osmosis/v21/x/lockup/types"
)

// This test validates that the FilterLocksByMinDuration function
// copies the correct locks when filtering.
// It helps to ensure that we do not use a wrong pointer by referencing
// a loop variable.
func TestFilterLocksByMinDuration(t *testing.T) {
const (
numLocks = 3
minDuration = 2
)

locks := make([]lockuptypes.PeriodLock, numLocks)
for i := 0; i < numLocks; i++ {
locks[i] = lockuptypes.PeriodLock{
ID: uint64(i + 1),
Duration: minDuration,
}
}

filteredLocks := keeper.FilterLocksByMinDuration(locks, minDuration)

require.Equal(t, len(locks), len(filteredLocks))

for i := 0; i < len(locks); i++ {
require.Equal(t, locks[i].ID, filteredLocks[i].ID)
}
}
2 changes: 1 addition & 1 deletion x/lockup/types/lock.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ func (p PeriodLock) SingleCoin() (sdk.Coin, error) {

// TODO: Can we use sumtree instead here?
// Assumes that caller is passing in locks that contain denom
func SumLocksByDenom(locks []PeriodLock, denom string) osmomath.Int {
func SumLocksByDenom(locks []*PeriodLock, denom string) osmomath.Int {
sumBi := big.NewInt(0)
// validate the denom once, so we can avoid the expensive validate check in the hot loop.
err := sdk.ValidateDenom(denom)
Expand Down