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

store/cachekv: newMemIterator should use list.Len() to retrieve the slice size and cut out append activity #8795

Closed
3 of 4 tasks
odeke-em opened this issue Mar 5, 2021 · 0 comments · Fixed by #8796
Closed
3 of 4 tasks
Assignees
Labels
T: Performance Performance improvements

Comments

@odeke-em
Copy link
Collaborator

odeke-em commented Mar 5, 2021

Summary of Bug

While profiling to fix #8747, while examining many profiles for a bunch, over 53 seconds, noticed this code

func newMemIterator(start, end []byte, items *list.List, ascending bool) *memIterator {
itemsInDomain := make([]*kv.Pair, 0)

if we look at profiles

ROUTINE ======================== github.com/cosmos/cosmos-sdk/store/cachekv.newMemIterator in /Users/emmanuelodeke/go/src/github.com/cosmos/cosmos-sdk/store/cachekv/memiterator.go
     3.89s      8.09s (flat, cum) 15.02% of Total
         .          .     21:func newMemIterator(start, end []byte, items *list.List, ascending bool) *memIterator {
         .          .     22:	itemsInDomain := make([]*kv.Pair, 0)
         .          .     23:
         .          .     24:	var entered bool
         .          .     25:
     160ms      440ms     26:	for e := items.Front(); e != nil; e = e.Next() {
     1.47s      1.48s     27:		item := e.Value.(*kv.Pair)
     1.93s      2.96s     28:		if !dbm.IsKeyInDomain(item.Key, start, end) {
         .          .     29:			if entered {
         .          .     30:				break
         .          .     31:			}
         .          .     32:
         .          .     33:			continue
         .          .     34:		}
         .          .     35:
     330ms      3.20s     36:		itemsInDomain = append(itemsInDomain, item)
         .          .     37:		entered = true
         .          .     38:	}
         .          .     39:
         .       10ms     40:	return &memIterator{
         .          .     41:		start:     start,
         .          .     42:		end:       end,
         .          .     43:		items:     itemsInDomain,
         .          .     44:		ascending: ascending,
         .          .     45:	}

The culprit to focus on is

     330ms      3.20s     36:		itemsInDomain = append(itemsInDomain, item)

cumulatively it took 3.20 seconds, but really items is a container/list.List which has a property https://golang.org/pkg/container/list/#List.Len which when used shaves down time from 3.20 seconds down to 600ms

ROUTINE ======================== github.com/cosmos/cosmos-sdk/store/cachekv.newMemIterator in /Users/emmanuelodeke/go/src/github.com/cosmos/cosmos-sdk/store/cachekv/memiterator.go
     5.61s      7.91s (flat, cum) 17.74% of Total
         .          .     17:	items      []*kv.Pair
         .          .     18:	ascending  bool
         .          .     19:}
         .          .     20:
         .          .     21:func newMemIterator(start, end []byte, items *list.List, ascending bool) *memIterator {
         .      260ms     22:	itemsInDomain := make([]*kv.Pair, 0, items.Len())
         .          .     23:
         .          .     24:	var entered bool
         .          .     25:
     190ms      410ms     26:	for e := items.Front(); e != nil; e = e.Next() {
     2.28s      2.30s     27:		item := e.Value.(*kv.Pair)
     2.61s      4.06s     28:		if !dbm.IsKeyInDomain(item.Key, start, end) {
      40ms       40ms     29:			if entered {
         .          .     30:				break
         .          .     31:			}
         .          .     32:
         .          .     33:			continue
         .          .     34:		}
         .          .     35:
     490ms      600ms     36:		itemsInDomain = append(itemsInDomain, item)
         .          .     37:		entered = true
         .          .     38:	}
         .          .     39:
         .      240ms     40:	return &memIterator{
         .          .     41:		start:     start,
         .          .     42:		end:       end,
         .          .     43:		items:     itemsInDomain,
         .          .     44:		ascending: ascending,
         .          .     45:	}

Version

Latest at 432b0b2

It is 4:25AM, and I've been on a long performance hunt, but I need to go to bed, but kindly delegating to other folks to take a look, the PR is pretty straightforward to get in.


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@odeke-em odeke-em added the T: Performance Performance improvements label Mar 5, 2021
@tac0turtle tac0turtle mentioned this issue Mar 5, 2021
9 tasks
@mergify mergify bot closed this as completed in #8796 Mar 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T: Performance Performance improvements
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants