-
Notifications
You must be signed in to change notification settings - Fork 461
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
Perf/dont redownload downloaded code #6873
Conversation
@@ -29,6 +30,9 @@ public class SnapProvider : ISnapProvider | |||
|
|||
private readonly ProgressTracker _progressTracker; | |||
|
|||
// This is actually close to 97% effective. | |||
private readonly LruKeyCache<ValueHash256> _codeExistKeyCache = new(1024 * 16, ""); |
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.
512kb of Keccaks; does/can this get dumped after sync?
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.
I think we can drop/clean it after the sync finishes.
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.
We found lately with @obasekiosa that acquiring lock in LRU can take quite some time in very congested scenarios. Would be good to create LRU that can avoid waiting for locks or maybe avoid LRU altogether and use NonBlocking.ConcurrentDictionary as hash?
I assume shouldn't be that contended here? |
This is in parallel: https://github.com/NethermindEth/nethermind/pull/6873/files#diff-59cbf3228de3b06fc97ff45e63cc32f17a5194574fe42e34ccf863515adb1b17R96-R103 and could produce high lock contention. It is fine for now - not blocking this PR, I will try to propose a nonblocking LRU cache :) |
@@ -205,6 +205,7 @@ public override void SyncModeSelectorOnChanged(SyncMode current) | |||
if (_disposed) return; | |||
if (CurrentState == SyncFeedState.Dormant) | |||
{ | |||
_snapProvider.Dispose(); |
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.
won't this be called over and over again?
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.
Yes.
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.
Ow, I think I made a mistake here.
Changes
Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?