-
Notifications
You must be signed in to change notification settings - Fork 89
Fix OutOfMemory in read stream #38
Fix OutOfMemory in read stream #38
Conversation
Hi, great, thanks for this extensive PR! 😺 I will try to have a look into this next week, currently at EthCC (if you are as well we can also meet). Can you rebase and squash the first 7 commits (be7ae to eece4) into the first (be7ae) commit, and also squash together the last two commits, these are mostly back-and-forth commits and bloat the commit history (if you haven't use rebase functionality before you should do a local copy of your repository directory since things can go wrong). |
Process would be: update your (if there are any |
@holgerd77 Done! Unfortunately, I'm not at EthCC. |
Beautiful. 👍 Otherwise this is ready for review? |
Yep, ready :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I now managed to get the example from @ivica7 running on my local geth database. Also had a look at the code line-by-line and for understanding what it does.
Since all the tests are also passing, I think I can give this a go now.
Really beautiful PR, thanks again! 🌻 😄
Just did the associated release on npm: https://github.com/ethereumjs/merkle-patricia-tree/releases/tag/v2.3.1 |
Fix for #36
The root cause of the issue is the trie is traversed asynchronously, in such a way that nodes on different levels are explored with equal probability, resembling breadth-first search. This requires the bulk of the trie to be loaded into memory before any entries can be emitted by the stream.
This pull request adds
PrioritizedTaskExecutor
with a pool and a priority queue, node lookups are prioritised by node key length. This resembles depth-first search, which solves the issue.New code is covered in
test/prioritizedTaskExecutor.js
Also tested here https://github.com/medvedev1088/merkle-patricia-tree/pull/1