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: Define a general Filter struct #1073

Merged
merged 1 commit into from
Jun 24, 2019

Conversation

keroro520
Copy link
Contributor

@keroro520 keroro520 commented Jun 19, 2019

  • refactor: Define a Filter struct

  • feat: Use LruCache to avoid memory leaking

    If we use native HashMap/HashSet as cache/filter, we have to prevent memory overflow by timely check or else. Replacing HashMap/HashSet with LruCache help us limit the memory usage, and reduce works around pruning staled items.

@keroro520 keroro520 requested a review from a team June 19, 2019 16:07
@nervos-bot
Copy link

nervos-bot bot commented Jun 19, 2019

@xxuejie is assigned as the chief reviewer

@xxuejie
Copy link
Collaborator

xxuejie commented Jun 20, 2019

Have we benched this? My concern is that HashMap (well, std's one is slow but the argument holds) is an optimized architecture with performance guarantees, with newer version of Rust they will get faster with new implementations such as hashbrown.

On the other hand, it seems like we don't need strict LRU policy here, in that case adding an LruCache might bring in a lot of unneeded logic. I feel it's better if we just cap the size of the HashMap till we know for sure this won't bring any negative effects.

sync/src/types.rs Show resolved Hide resolved
@u2
Copy link
Contributor

u2 commented Jun 20, 2019

In the implementation here, it's not an LRU, just a LinkedHashMap or LinkedHashSet with a capacity.

@u2
Copy link
Contributor

u2 commented Jun 20, 2019

Why I am authorized to close the PR?

@keroro520
Copy link
Contributor Author

Have we benched this? My concern is that HashMap (well, std's one is slow but the argument holds) is an optimized architecture with performance guarantees, with newer version of Rust they will get faster with new implementations such as hashbrown.

On the other hand, it seems like we don't need strict LRU policy here, in that case adding an LruCache might bring in a lot of unneeded logic. I feel it's better if we just cap the size of the HashMap till we know for sure this won't bring any negative effects.

Thanks!

Let me describe the background of this PR. Recently I am considering "spam attacks", the malicious send lots of spam requests/blocks/transactions. These spams will eat lots of memory since we don't prune timely. So I replace HashMap with LruCache. With the help of the feature "limited capacity" of LruCache, we can prevent the above spam attacks.

I haven't bench LruCache vs HashMap, and may not bench it later since I am gonna revert the change "HashMap -> LruCache".

After reading your comment, I review the two fields pending_compact_blocks and header_map, which is changed from HashMap to LruCache in this PR. And find that these two fields will never suffer from spam attacks since we always verify the header's PoW via HeaderVerifier before inserting into the cache.

So I am gonna revert 93a6def018cb7b7966c7df6063fcf625815578a3. Maybe we can keep the first commit and treat it as a refactoring PR?

cc @u2

@keroro520 keroro520 changed the title feat: Replace HashMap with LruCache refactor: Define a general Filter struct Jun 20, 2019
@u2
Copy link
Contributor

u2 commented Jun 20, 2019

lgtm

@zhangsoledad zhangsoledad merged commit b330d60 into nervosnetwork:develop Jun 24, 2019
@doitian doitian mentioned this pull request Jul 4, 2019
1 task
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.

5 participants