perf: improve CL KVStore entries #5205

czarcas7ic · 2023-05-17T18:11:09Z

Closes: #4733

What is the purpose of the change

The current Owner | Pool ID | Position ID -> Position ID KVStore returns values like so:

addr1|1|1->1
addr1|1|2->2
addr1|2|3->3
addr2|1|4->4
addr2|1|5->5

This PR instead keys Owner | Pool ID -> list of Position IDs like so:

addr1|1|->[1, 2]
addr1|2|->[3]
addr2|1|->[4,5]

Similarly, this PR keys Pool ID -> list of Position IDs like so:

pool1->[1,3,5,6]
pool2->[2,4,7]

Testing and Verifying

DeletePosition and CreatePosition tests modified to expect positionIds returned from the KVStore
Osmoutils additions (SliceContains, RemoveFromSlice, BigEndianToUint64Slice, and Uint64SliceToBigEndian) have corresponding tests

Documentation and Release Note

Does this pull request introduce a new feature or user-facing behavior changes?
Changelog entry added to Unreleased section of CHANGELOG.md?

Where is the change documented?

Specification (x/{module}/README.md)
Osmosis documentation site
Code comments?
N/A

p0mvn · 2023-05-17T18:13:05Z

oh perfect! I was going to start working on this today for further perf improvements

czarcas7ic · 2023-05-17T18:14:00Z

Yeah, was not super hard to do and feels like it will save a ton of space :)

Still need to add tests for the new osmoutils funcs but should be R4R soon

p0mvn

I'm worried about doing this change at this stage of the project lifecycle.

When I commented first, I thought that this perf change was about removing Sprintf from keys. It's questionable whether there are even performance improvements since letting the storage search for keys is usually more efficient than doing it in the application layer. Additionally, getting all position IDs from store on set and delete is questionable.

Overall, the benefits of this change are negligible but the risk is high, making it a timeline risk.

For me to approve it, I would like to see a benchmark that this is making a worthwhile perf improvement

p0mvn · 2023-05-17T20:51:31Z

x/concentrated-liquidity/position.go

+	positionIds := []uint64{}
+	addressPoolIdKey := types.KeyAddressAndPoolId(sender, poolId)
+	// Get the existing position IDs for the given address-pool ID.
+	positionIdsBytes := store.Get(addressPoolIdKey)
+	if len(positionIdsBytes) > 0 {
+		positionIds = osmoutils.BigEndianToUint64Slice(positionIdsBytes)
 	}
-	isOwner, err := osmoutils.HasAnyAtPrefix(ctx.KVStore(k.storeKey), types.KeyAddressPoolIdPositionId(sender, poolId, positionId), parse)
-	if err != nil {
-		return false, err
+	// Check if the given positionId is in the list of existing position IDs.
+	if osmoutils.SliceContains(positionIds, positionId) {


At first, I was slightly worried about this approach. However, on another look seems fine

Assume someone has a million positions in pool. Then,

8 * 1000000 / 1024 / 1024 = 7.6 MB in memory

Since we are processing messages sequentially it's unlikely this will cause a problem

p0mvn · 2023-05-17T20:56:16Z

x/concentrated-liquidity/position.go

-	positionIds, err := osmoutils.GatherValuesFromStorePrefix(ctx.KVStore(k.storeKey), prefix, ParsePositionIdFromBz)
+	positionIds, err := osmoutils.GatherValuesFromStorePrefix(ctx.KVStore(k.storeKey), prefix, ParsePositionIdsFromBz)
 	if err != nil {
 		return nil, err
 	}

 	// Retrieve each position from the store using its ID and add it to the result slice.
-	for _, positionId := range positionIds {
-		position, err := k.GetPosition(ctx, positionId)
-		if err != nil {
-			return nil, err
+	for _, poolPositionIds := range positionIds {
+		for _, positionId := range poolPositionIds {
+			position, err := k.GetPosition(ctx, positionId)
+			if err != nil {
+				return nil, err
+			}
+			positions = append(positions, position)


I think I understand what's happening here but it's quite unclear why we need to iterate over positionIds and then poolPositionIds. Can you please update this to make it more readable?

czarcas7ic · 2023-05-17T21:18:10Z

@p0mvn I think its less of a performance change and more of a storage change. The current keys feel silly / redundant. We also have informal starting their audit tomorrow, so this would be a part of the scope. IMO I think if we don't get this in now we will never get it in, and we will have a bunch of silly redundant state that is not needed.

mattverse · 2023-05-18T05:31:12Z

osmoutils/store_helper.go

+}
+
+// RemoveFromSlice removes a uint64 value from a slice and returns the new slice.
+// It returns an error if the value is not found in the slice.


Suggested change

// It returns an error if the value is not found in the slice.

// It returns an error if the value is not found in the slice.

// The computation cost of this method is O(n) as we linearly iterate over given slice

mattverse · 2023-05-18T06:28:55Z

x/concentrated-liquidity/position.go

@@ -93,25 +93,37 @@ func (k Keeper) HasAnyPositionForPool(ctx sdk.Context, poolId uint64) (bool, err
 }

 // isPositionOwner returns true if the given positionId is owned by the given sender inside the given pool.


Suggested change

// isPositionOwner returns true if the given positionId is owned by the given sender inside the given pool.

// isPositionOwner returns true if the given positionId is owned by the given sender inside the given pool.

// This method also returns false if the given position is not found from state.

mattverse · 2023-05-18T06:37:58Z

x/concentrated-liquidity/position.go

@@ -154,18 +166,20 @@ func (k Keeper) GetUserPositions(ctx sdk.Context, addr sdk.AccAddress, poolId ui
 	positions := []model.Position{}

 	// Gather all position IDs for the given user and pool ID.
-	positionIds, err := osmoutils.GatherValuesFromStorePrefix(ctx.KVStore(k.storeKey), prefix, ParsePositionIdFromBz)
+	positionIds, err := osmoutils.GatherValuesFromStorePrefix(ctx.KVStore(k.storeKey), prefix, ParsePositionIdsFromBz)


Perhaps a variable rename might make this less confusing, perhaps using poolPositionIds here

mattverse · 2023-05-18T06:45:13Z

x/concentrated-liquidity/position.go

+	if len(positionIdsBytes) > 0 {
+		positionIds = osmoutils.BigEndianToUint64Slice(positionIdsBytes)
+	}
+	// Add the new position ID to the existing position IDs only if it doesn't exist already.


What is the case that the position ID we are trying to add is already existing?

mattverse · 2023-05-18T06:45:56Z

x/concentrated-liquidity/position.go

+	if len(positionIdsBytes) > 0 {
+		positionIds = osmoutils.BigEndianToUint64Slice(positionIdsBytes)
+	}
+	// Add the new position ID to the existing position IDs only if it doesn't exist already.


mattverse · 2023-05-18T06:47:26Z

x/concentrated-liquidity/position.go

 	// Set the address-pool-position ID to position mapping.
-	addressPoolIdPositionIdKey := types.KeyAddressPoolIdPositionId(owner, poolId, positionId)
-	store.Set(addressPoolIdPositionIdKey, sdk.Uint64ToBigEndian(positionId))
+	positionIds := []uint64{}


I think go doc for this method needs to be updated accordingly

mattverse · 2023-05-18T07:06:53Z

@p0mvn I think its less of a performance change and more of a storage change. The current keys feel silly / redundant. We also have informal starting their audit tomorrow, so this would be a part of the scope. IMO I think if we don't get this in now we will never get it in, and we will have a bunch of silly redundant state that is not needed.

Sorry, I think I might be lacking context. What or how does this improve existing way of storing state? At a brief glance, this PR seems more like style change more than storage efficiency change, I'm pretty sure I am missing some details or links in understanding this change

czarcas7ic · 2023-05-19T20:48:02Z

As discussed in call, closing this in favor of #5237

Thanks for all the reviews, was still helpful in getting to our final result :)

improve cl kvstore entries

78500ca

czarcas7ic added V:state/breaking State machine breaking PR no-changelog labels May 17, 2023

github-actions bot added the C:x/concentrated-liquidity label May 17, 2023

czarcas7ic added 2 commits May 17, 2023 13:15

update osmoutils gomod

aee2b49

add osmoutils tests

ac71ca8

czarcas7ic added no-changelog V:state/breaking State machine breaking PR and removed V:state/breaking State machine breaking PR no-changelog labels May 17, 2023

czarcas7ic changed the title ~~refactor: improve CL KVStore entries~~ perf: improve CL KVStore entries May 17, 2023

czarcas7ic added 3 commits May 17, 2023 13:36

Merge branch 'main' into adam/cl-kvstore-perf

13ad4c9

merge main conflicts

6c44570

Merge branch 'main' into adam/cl-kvstore-perf

b8675d7

czarcas7ic added A:no-changelog and removed no-changelog labels May 17, 2023

update test

c8c35ab

github-actions bot added the C:simulator Edits simulator or simulations label May 17, 2023

czarcas7ic added 3 commits May 17, 2023 15:04

fix parse

3ca220a

add test for remove from slice

4d1778f

update gomod for osmoutils

3c3588d

czarcas7ic marked this pull request as ready for review May 17, 2023 20:43

czarcas7ic requested a review from p0mvn as a code owner May 17, 2023 20:43

update in line comment

acfde1c

p0mvn reviewed May 17, 2023

View reviewed changes

mattverse reviewed May 18, 2023

View reviewed changes

czarcas7ic marked this pull request as draft May 19, 2023 16:26

czarcas7ic closed this May 19, 2023

czarcas7ic deleted the adam/cl-kvstore-perf branch August 7, 2023 23:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf: improve CL KVStore entries #5205

perf: improve CL KVStore entries #5205

czarcas7ic commented May 17, 2023 •

edited

Loading

p0mvn commented May 17, 2023

czarcas7ic commented May 17, 2023 •

edited

Loading

p0mvn left a comment

p0mvn May 17, 2023

p0mvn May 17, 2023

czarcas7ic commented May 17, 2023

mattverse May 18, 2023

mattverse May 18, 2023

mattverse May 18, 2023

mattverse May 18, 2023

mattverse May 18, 2023

mattverse May 18, 2023

mattverse commented May 18, 2023 •

edited

Loading

czarcas7ic commented May 19, 2023

	// It returns an error if the value is not found in the slice.
	// It returns an error if the value is not found in the slice.
	// The computation cost of this method is O(n) as we linearly iterate over given slice

		@@ -93,25 +93,37 @@ func (k Keeper) HasAnyPositionForPool(ctx sdk.Context, poolId uint64) (bool, err
		}

		// isPositionOwner returns true if the given positionId is owned by the given sender inside the given pool.

	// isPositionOwner returns true if the given positionId is owned by the given sender inside the given pool.
	// isPositionOwner returns true if the given positionId is owned by the given sender inside the given pool.
	// This method also returns false if the given position is not found from state.

perf: improve CL KVStore entries #5205

perf: improve CL KVStore entries #5205

Conversation

czarcas7ic commented May 17, 2023 • edited Loading

What is the purpose of the change

Testing and Verifying

Documentation and Release Note

p0mvn commented May 17, 2023

czarcas7ic commented May 17, 2023 • edited Loading

p0mvn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

czarcas7ic commented May 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattverse commented May 18, 2023 • edited Loading

czarcas7ic commented May 19, 2023

czarcas7ic commented May 17, 2023 •

edited

Loading

czarcas7ic commented May 17, 2023 •

edited

Loading

mattverse commented May 18, 2023 •

edited

Loading