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

DVT-1057 replace block data structure with LRU cache to fix memory leak #158

Merged
merged 3 commits into from
Nov 14, 2023

Conversation

gatsbyz
Copy link
Contributor

@gatsbyz gatsbyz commented Nov 14, 2023

  • implement LRU cache for
    • efficient memory usage
    • preventing memory leaks by limiting the number of blocks in memory
  • ensure application scalability by enabling polycli monitor to run indefinitely without running out of memory.
  • improve performance by automatically evicting least accessed blocks, focusing resources on recent and relevant data.

added functions

  • isBlockInCache: Determines if a given block number is already available in the LRU cache to avoid unnecessary network requests.
  • checkAndFetchMissingBlocks: Identifies blocks not present in the cache within a specified range and fetches them from the Ethereum network to fill the cache.

removed

  • remove currIdx, selectedBlock, Max/MinBlockRetrieved to clean up logic variables

fix after PR was reverted. Add back LRU lock when being read/written to prevent race condition.

@gatsbyz gatsbyz changed the title Jesse/fix memory leak @gatsbyz DVT-1057 replace block data structure with LRU cache to fix memory leak Nov 14, 2023
@gatsbyz gatsbyz changed the title @gatsbyz DVT-1057 replace block data structure with LRU cache to fix memory leak DVT-1057 replace block data structure with LRU cache to fix memory leak Nov 14, 2023
}
return nil
}
const maxDataPoints = 1000
Copy link
Member

Choose a reason for hiding this comment

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

I think we should put this value at the top of the file with a comment that explains exactly what it does. I can imagine myself checking the code in 3 months wondering what this is about :)

blms = append(blms, ethrpc.BatchElem{
Method: "eth_getBlockByNumber",
Args: []interface{}{"0x" + i.Text(16), true},
Result: r,
Error: err,
Error: nil,
Copy link
Member

Choose a reason for hiding this comment

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

nice catch

Copy link
Member

@leovct leovct left a comment

Choose a reason for hiding this comment

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

I didn't review all the code as there are many changes in monitor.go but I went trough the code quickly. I tested and it looks good to me.

Side note: last PR, polycli monitor was working fine for me and not crashing at all (which is weird) but not for others so maybe wait for another review. :)

@gatsbyz gatsbyz merged commit 4f69f6e into main Nov 14, 2023
6 checks passed
@gatsbyz gatsbyz deleted the jesse/fix-memory-leak branch November 14, 2023 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants