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

(COMMON): Basic (Least Recently Used) LRU caching system #420

Merged
merged 13 commits into from Jan 24, 2020

Conversation

xtda
Copy link
Contributor

@xtda xtda commented Jan 22, 2020

PR Description

This PR adds a basic least recently used cache system that will be used as part of #409 (and others parts if people find it useful)

But as I learned my lesson with #383 and its 15,000 changes I decided to break it up into smaller PR's for additional non-directly related functionality

LRU is a type of caching style that stores elements and moves the most recently accessed to the front and disgards the least used if it would grow beyond capacity

There is both a concurrent safe( with mutex)

func New(capacity uint64) *LRUCache {
	return &LRUCache{
		lru: NewLRUCache(capacity),
	}
}

or called directly for non-concurrent safe version:

func NewLRUCache(capacity uint64) *LRU {
	return &LRU{
		Cap:   capacity,
		List:  list.New(),
		Items: make(map[interface{}]*list.Element),
	}
}

Obviously up to user to decide on requirements as if they need it to be goroutine safe or not and performance of mutexs

Type of change

Please delete options that are not relevant and add an x in [] as item is complete.

  • New feature (non-breaking change which adds functionality)

How has this been tested

  • go test ./... -race
  • golangci-lint run

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation and regenerated documentation via the documentation tool
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally and on Travis with my changes
  • Any dependent changes have been merged and published in downstream modules

@xtda xtda added enhancement review me This pull request is ready for review labels Jan 22, 2020
@xtda xtda self-assigned this Jan 22, 2020
@xtda
Copy link
Contributor Author

xtda commented Jan 22, 2020

Also I have not added the ability to "Peek" (get a keys value without moving it to the front of the cache) or List all entires as I personally feel this goes against the point of of a LRU cache as you are accessing it and it was recently used

Happy to add if people want this functionality

@codecov
Copy link

codecov bot commented Jan 23, 2020

Codecov Report

Merging #420 into master will increase coverage by 0.21%.
The diff coverage is 43.81%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #420      +/-   ##
==========================================
+ Coverage   41.12%   41.33%   +0.21%     
==========================================
  Files         164      182      +18     
  Lines       38678    41950    +3272     
==========================================
+ Hits        15906    17340    +1434     
- Misses      21639    23167    +1528     
- Partials     1133     1443     +310
Impacted Files Coverage Δ
engine/orders.go 0% <ø> (ø) ⬆️
logger/logger_types.go 100% <ø> (ø) ⬆️
config/config_types.go 79.41% <ø> (ø) ⬆️
engine/gctscript.go 0% <0%> (ø)
database/models/postgres/script_execution.go 0% <0%> (ø)
engine/rpcserver.go 0% <0%> (ø) ⬆️
engine/engine.go 0% <0%> (ø) ⬆️
common/cache/cache.go 100% <100%> (ø) ⬆️
gctscript/wrappers/validator/validator.go 100% <100%> (ø)
gctscript/vm/vm_error.go 100% <100%> (ø)
... and 40 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 71cbfad...f7c1a81. Read the comment docs.

@thrasher- thrasher- changed the title (COMMON): Basic LRU caching system (COMMON): Basic (Least Recently Used) LRU caching system Jan 23, 2020
Copy link
Collaborator

@thrasher- thrasher- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome stuff! Great first caching algo with many more to come (FIFO/LIFO etc). Just a few minor nits

common/cache/README.md Outdated Show resolved Hide resolved
common/cache/README.md Outdated Show resolved Hide resolved
common/cache/cache_types.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@shazbert shazbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no nits, except what Thrasher has suggested. 👍

Copy link
Collaborator

@thrasher- thrasher- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making those changes!

Copy link
Collaborator

@gloriousCode gloriousCode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So nice and clean. Very nice 🎉 Just a few minor observations and requests.

common/cache/cache_test.go Outdated Show resolved Hide resolved
common/cache/cache_test.go Outdated Show resolved Hide resolved
common/cache/cache_test.go Show resolved Hide resolved
common/cache/lru.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@gloriousCode gloriousCode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making those changes! Also confirmed it works concurrently. Looking forward to seeing its use

Copy link
Collaborator

@thrasher- thrasher- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last minor nits just associated with comments to keep golint happy and to match them up

common/cache/cache.go Show resolved Hide resolved
return false
}

// Contain checks if cache contains key
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// Contains

return l.lru.getNewest()
}

// Contain checks if cache contains key if not adds to cache
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// ContainsOrAdd checks if cache contains key and if not, adds it to the cache

return l.lru.getOldest()
}

// Get looks up a key's value from the cache.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This returns the newest k,v from the LRU

Copy link
Collaborator

@shazbert shazbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an awesome addition well done!

@thrasher- thrasher- merged commit e5b64a5 into thrasher-corp:master Jan 24, 2020
@xtda xtda deleted the cache branch February 9, 2020 21:35
MadCozBadd pushed a commit to MadCozBadd/gocryptotrader that referenced this pull request Mar 24, 2020
…rp#420)

* started adding basic lru cache system

* Added basic LRU cache including Add Get Remove Contains ContainsOrAdd Clear

* wording changes on comments

* removed exported var's in strut as they are not required

* Added README

* README updates

* rm line :D

* swapped to mutex over rwmutex updated comments

* unexported getNewest & getOldest

* unexported getNewest & getOldest

* Updated comments and cited references in source

* updated comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement review me This pull request is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants