mempool: LRUTxCache.Reset could benefit from using the map clearing idiom to reduce CPU & RAM pressure #2243

odeke-em · 2024-02-05T22:13:43Z

If we look at this code

Lines 56 to 62 in f92bace

    
           func (c *LRUTxCache) Reset() { 
        
           	c.mtx.Lock() 
        
           	defer c.mtx.Unlock() 
        
           	c.cacheMap = make(map[types.TxKey]*list.Element, c.size) 
        
           	c.list.Init() 
        
           }

we can see that .Reset simply takes a lock then allocates a fresh map by discarding the previous one.

Suggestion

We can reduce on CPU & RAM pressure by using the map clearing idiom aka

func (c *LRUTxCache) Reset() { 
 	c.mtx.Lock() 
 	defer c.mtx.Unlock() 
  
        for key := range c.cacheMap {
             delete(c.cacheMap, key)
 	}
 	c.list.Init() 
 }

which in performance wisdom is a known and accepted Go idiom for reducing CPU & RAM pressure by reusing the map.

melekes · 2024-02-07T05:35:21Z

https://tip.golang.org/ref/spec#Clear

func (c *LRUTxCache) Reset() { 
...  
         clear(c.cacheMap)
         c.list.Init() 
 }

instead of allocating a new one Closes #2243

odeke-em · 2024-02-07T05:44:28Z

@melekes clear was introduced in Go1.21, is every user of this repository already upgraded to that very recent Go version?

melekes · 2024-02-07T08:08:24Z

https://github.com/cometbft/cometbft?tab=readme-ov-file#minimum-requirements

don't know about every user, but our current min version is 1.21 as stated in the README

instead of allocating a new one Closes #2243 It should not be backported onto <= v0.37 because their min Go version is 1.20, and `clear` was introduced in Go 1.21. --- #### PR checklist - [ ] Tests written/updated - [ ] ~~Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog)~~ - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments - [x] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec

instead of allocating a new one Closes #2243 It should not be backported onto <= v0.37 because their min Go version is 1.20, and `clear` was introduced in Go 1.21. --- #### PR checklist - [ ] Tests written/updated - [ ] ~~Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog)~~ - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments - [x] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec (cherry picked from commit fc4e1ef)

melekes added a commit that referenced this issue Feb 7, 2024

perf(mempool/cache): clear map in Reset

1095068

instead of allocating a new one Closes #2243

melekes mentioned this issue Feb 7, 2024

perf(mempool/cache): clear map in Reset #2259

Merged

4 tasks

melekes closed this as completed in #2259 Feb 9, 2024

adizere added this to the 2024-Q1 milestone Mar 1, 2024

adizere added this to CometBFT Mar 1, 2024

github-project-automation bot moved this to Todo in CometBFT Mar 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

mempool: LRUTxCache.Reset could benefit from using the map clearing idiom to reduce CPU & RAM pressure #2243

mempool: LRUTxCache.Reset could benefit from using the map clearing idiom to reduce CPU & RAM pressure #2243

Uh oh!

Uh oh!

Uh oh!

mempool: LRUTxCache.Reset could benefit from using the map clearing idiom to reduce CPU & RAM pressure #2243

mempool: LRUTxCache.Reset could benefit from using the map clearing idiom to reduce CPU & RAM pressure #2243

Comments

Suggestion

8000 Uh oh!

Uh oh!

Uh oh!

Uh oh!