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

Refactor fastNodeCache and nodeCache to be separate modules with synchrony #24

Closed
9 tasks
Tracked by #1016 ...
p0mvn opened this issue Feb 21, 2022 · 0 comments · Fixed by #38
Closed
9 tasks
Tracked by #1016 ...

Refactor fastNodeCache and nodeCache to be separate modules with synchrony #24

p0mvn opened this issue Feb 21, 2022 · 0 comments · Fixed by #38
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed tech-debt

Comments

@p0mvn
Copy link
Member

p0mvn commented Feb 21, 2022

Background

fastNodeCache and nodeCache in nodeDb are resources accessible by several go routines.

There are 2 problems with the current design:

  1. Each cache has 3 elements:
  • cache
  • cacheSize
  • cacheQueue

When we update one, we need to update all other 3. The current implementation makes the code less readable since we need to update all 3 and many places in nodedb.

Background

fastNodeCache and nodeCache in nodeDb are resources accessible by several go routines that persist nodes in an LRU fashion.

Problem
There are 2 problems with the current design:

  1. Each cache has 3 elements:
  • cache
  • cacheSize
  • cacheQueue

When we update one, we need to update all other 3. The current implementation makes the code less readable since we need to update all 3 in many places in nodedb. ​

  1. There is only one mutex for all common resources in nodedb.

Sometimes we might be reading resource A while using the common lock. At the same time, another goroutine wants to read resource B but has to wait on that exact same lock

  1. It is difficult to unit test.

Solution

Instead, we can have an interface:

type NodeCache interface{
   Get(key []byte) (node *Node, ok bool) // ok is true if node exists, similar to accessing a map in Go
   Cache(node *Node)
   Uncache(key []byte)
} 

This way, we would be able to decouple cache from nodedb, improve the readability of the code and make it less brittle.

In addition, by decoupling NodeCache and letting each has its own mutex, we will solve the performance issue of having to wait on the common lock while accessing different resources

Lastly, by having a NodeCache interface, we can easily mock it with gomock

Before You Start

This issue is blocked by #24 , #24 should be done before this issue

Acceptance Criteria

  • create NodeCache interface
  • create TreeNodeCache struct (see Refactor fastNodeCache and nodeCache to be separate modules with synchrony #24 for what TreeNode is)
  • unit test TreeNodeCache
  • mock the cache and add more unit tests for nodedb with the mock
  • create FastNodeCache struct
  • unit test TreeNodeCache
  • mock the cache and add more unit tests for nodedb with the mock
  • additional unit tests are added where needed
  • all unit tests pass
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed tech-debt
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

1 participant