patch 9: improve performance when working with slices

x/dymns/keeper/generic_reverse_lookup.go

@@ -1,8 +1,10 @@
 package keeper
 
 import (
+	"slices"
+	"strings"
+
 	sdk "github.com/cosmos/cosmos-sdk/types"
-	dymnstypes "github.com/dymensionxyz/dymension/v3/x/dymns/types"
 )
 
 // GenericAddReverseLookupRecord is a utility method that help to add a reverse lookup record.
@@ -12,26 +14,22 @@ func (k Keeper) GenericAddReverseLookupRecord(
 	marshaller func([]string) []byte,
 	unMarshaller func([]byte) []string,
 ) error {
-	modifiedRecord := dymnstypes.StringList{
-		newElement,
-	}
+	var modifiedRecord []string
 
 	store := ctx.KVStore(k.storeKey)
 	bz := store.Get(key)
 	if bz != nil {
 		existingRecord := unMarshaller(bz)
 
-		modifiedRecord = dymnstypes.StringList(existingRecord).Combine(
-			modifiedRecord,
-		)
-
-		if len(modifiedRecord) == len(existingRecord) {
-			// no new mapping to add
+		if slices.Contains(existingRecord, newElement) {
+			// already exist
 			return nil
 		}
-	}
 
-	modifiedRecord = modifiedRecord.Sort()
+		modifiedRecord = append(existingRecord, newElement)
+	} else {
+		modifiedRecord = []string{newElement}
+	}
 
 	bz = marshaller(modifiedRecord)
 	store.Set(key, bz)
@@ -69,16 +67,25 @@ func (k Keeper) GenericRemoveReverseLookupRecord(
 	}
 
 	existingRecord := unMarshaller(bz)
+	modifiedRecord := slices.DeleteFunc(existingRecord, func(r string) bool {
+		return r == elementToRemove
+	})
 
-	modifiedRecord := dymnstypes.StringList(existingRecord).Exclude([]string{elementToRemove})
+	if len(existingRecord) == len(modifiedRecord) {
+		// not found
+		return nil
+	}
 
 	if len(modifiedRecord) == 0 {
 		// no more, remove record
 		store.Delete(key)
 		return nil
 	}
 
-	modifiedRecord = modifiedRecord.Sort()
+	// just for safety, sort the records
+	slices.SortFunc(modifiedRecord, func(a, b string) int {
+		return strings.Compare(a, b)
+	})
 
 	bz = marshaller(modifiedRecord)
 	store.Set(key, bz)

x/dymns/keeper/generic_reverse_lookup_test.go

@@ -128,21 +128,6 @@ func (s *KeeperTestSuite) TestKeeper_GenericAddGetRemoveReverseLookupRecord() {
 				s.Equal([]string{"test", "test2"}, records) // still the same
 			},
 		},
-		{
-			name: "add - list must be sorted before persist",
-			testFunc: func(te testEntity, ctx sdk.Context, s *KeeperTestSuite) {
-				te.adder(ctx, keyTestReverseLookup, "test3", s)
-				te.adder(ctx, keyTestReverseLookup, "test", s)
-
-				records := te.getter(ctx, keyTestReverseLookup, s)
-				s.Equal([]string{"test", "test3"}, records)
-
-				te.adder(ctx, keyTestReverseLookup, "test2", s)
-
-				records = te.getter(ctx, keyTestReverseLookup, s)
-				s.Equal([]string{"test", "test2", "test3"}, records)
-			},
-		},
 		{
 			name: "get - returns empty when getting non-exist record",
 			testFunc: func(te testEntity, ctx sdk.Context, s *KeeperTestSuite) {
@@ -158,18 +143,18 @@ func (s *KeeperTestSuite) TestKeeper_GenericAddGetRemoveReverseLookupRecord() {
 				te.adder(ctx, keyTestReverseLookup, "test1", s)
 
 				records := te.getter(ctx, keyTestReverseLookup, s)
-				s.Equal([]string{"test1", "test2", "test3"}, records)
+				s.Equal([]string{"test3", "test2", "test1"}, records)
 			},
 		},
 		{
-			name: "get - result is ordered",
+			name: "get - result is kept as persist order",
 			testFunc: func(te testEntity, ctx sdk.Context, s *KeeperTestSuite) {
 				te.adder(ctx, keyTestReverseLookup, "test3", s)
 				te.adder(ctx, keyTestReverseLookup, "test2", s)
 				te.adder(ctx, keyTestReverseLookup, "test1", s)
 
 				records := te.getter(ctx, keyTestReverseLookup, s)
-				s.Equal([]string{"test1", "test2", "test3"}, records)
+				s.Equal([]string{"test3", "test2", "test1"}, records)
 			},
 		},
 		{
@@ -292,6 +277,10 @@ func Benchmark_GenericAddReverseLookupRecord(b *testing.B) {
 	// 2024-09-26: 0.43s for appending into a list of existing 1m elements
 	// Benchmark_GenericAddReverseLookupRecord-8 | 3s154ms | 3 | 431924139 ns/op | 272465408 B/op | 1038262 allocs/op
 
+	// 2024-09-26: 0.062s for appending into a list of existing 1m elements
+	// Benchmark_GenericAddReverseLookupRecord-8 | 2s619ms | 18 | 62825618 ns/op | 224076049 B/op | 1000054 allocs/op
+	// => After improve slice operations, the time needed per op reduced from 430ms to 62ms
+
 	s := new(KeeperTestSuite)
 	s.SetT(&testing.T{})
 	s.SetupTest()
@@ -343,6 +332,10 @@ func Benchmark_GenericRemoveReverseLookupRecord(b *testing.B) {
 	// 2024-09-26: 0.44s for removing from a list of existing 1m elements
 	// Benchmark_GenericRemoveReverseLookupRecord-8 | 3s471ms | 3 | 440934042 ns/op | 293803210 B/op | 1038246 allocs/op
 
+	// 2024-09-26: 0.1s for removing from a list of existing 1m elements
+	// Benchmark_GenericRemoveReverseLookupRecord-8 | 7s638ms | 12 | 102355965 ns/op | 224075756 B/op | 1000041 allocs/op
+	// => After improve slice operations, the time needed per op reduced from 430ms to 62ms
+
 	s := new(KeeperTestSuite)
 	s.SetT(&testing.T{})
 	s.SetupTest()

x/dymns/keeper/hooks.go

@@ -80,6 +80,8 @@ func (e epochHooks) processActiveDymNameSellOrders(ctx sdk.Context, logger log.L
 	logger.Info("processing finished SOs.", "count", len(finishedSOs))
 
 	for _, so := range finishedSOs {
+		// the execution order should be guaranteed because activeSellOrdersExpiration is sorted
+
 		// each order should be processed in a branched context, if error, discard the state change
 		// and process next order, to prevent chain reaction when an individual order failed to process
 		errApplyStateChange := osmoutils.ApplyFuncIfNoError(ctx, func(ctx sdk.Context) error {
@@ -132,6 +134,8 @@ func (e epochHooks) processActiveAliasSellOrders(ctx sdk.Context, logger log.Log
 	prohibitedToTradeAliases := e.GetAllAliasAndChainIdInParams(ctx)
 
 	for _, so := range finishedSOs {
+		// the execution order should be guaranteed because activeSellOrdersExpiration is sorted
+
 		// each order should be processed in a branched context, if error, discard the state change
 		// and process next order, to prevent chain reaction when an individual order failed to process
 		errApplyStateChange := osmoutils.ApplyFuncIfNoError(ctx, func(ctx sdk.Context) error {

x/dymns/keeper/sell_order_test.go

@@ -3,13 +3,14 @@ package keeper_test
 import (
 	cryptorand "crypto/rand"
 	"fmt"
-	testkeeper "github.com/dymensionxyz/dymension/v3/testutil/keeper"
-	"github.com/stretchr/testify/require"
 	"math/big"
 	"math/rand"
 	"testing"
 	"time"
 
+	testkeeper "github.com/dymensionxyz/dymension/v3/testutil/keeper"
+	"github.com/stretchr/testify/require"
+
 	"github.com/dymensionxyz/sdk-utils/utils/uptr"
 
 	sdk "github.com/cosmos/cosmos-sdk/types"

x/dymns/types/dym_name.go

@@ -2,7 +2,6 @@ package types
 
 import (
 	"fmt"
-	"sort"
 	"strings"
 
 	errorsmod "cosmossdk.io/errors"
@@ -273,33 +272,3 @@ func (m *DymName) GetAddressesForReverseMapping() (
 
 	return
 }
-
-// Distinct returns a new list of dym names with duplicates removed.
-// Result will be sorted.
-func (m ReverseLookupDymNames) Distinct() (distinct ReverseLookupDymNames) {
-	return ReverseLookupDymNames{
-		DymNames: StringList(m.DymNames).Distinct(),
-	}
-}
-
-// Combine merges the dym names from the current list and the other list.
-// Result will be sorted distinct.
-func (m ReverseLookupDymNames) Combine(other ReverseLookupDymNames) ReverseLookupDymNames {
-	return ReverseLookupDymNames{
-		DymNames: StringList(m.DymNames).Combine(other.DymNames),
-	}.Distinct()
-}
-
-// Exclude removes the dym names from the current list that are in the toBeExcluded list.
-// Result will be sorted distinct.
-func (m ReverseLookupDymNames) Exclude(toBeExcluded ReverseLookupDymNames) (afterExcluded ReverseLookupDymNames) {
-	return ReverseLookupDymNames{
-		DymNames: StringList(m.DymNames).Exclude(toBeExcluded.DymNames),
-	}.Distinct()
-}
-
-// Sort sorts the dym names in the list.
-func (m ReverseLookupDymNames) Sort() ReverseLookupDymNames {
-	sort.Strings(m.DymNames)
-	return m
-}