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

Perf/dont redownload downloaded code #6873

Merged
merged 7 commits into from
Mar 28, 2024

Conversation

asdacap
Copy link
Contributor

@asdacap asdacap commented Mar 27, 2024

  • Turns out a large portion of the incoming and outgoing portion of uncompressed snap message traffic are code which are duplicated.
  • This add a filter so that persisted codes are not redownloaded.
  • This reduces about 30% of uncompressed incoming traffic and about 50% of uncompressed outgoing traffic. Compressed traffic is likely much lower as these codes are largely the same and repeated.
  • Overall effect on snap sync is not much, although multiple run shows a slight improvement int statedb write rates.
  • Graph is (before, after)X5.
    Screenshot_2024-03-27_22-26-37

Changes

  • Check if a code was persisted, if so don't download it again.
  • Add an LRU to reduce IO.

Types of changes

What types of changes does your code introduce?

  • Optimization

Testing

Requires testing

  • Yes
  • No

If yes, did you write tests?

  • Yes
  • No

@@ -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, "");
Copy link
Member

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?

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 can drop/clean it after the sync finishes.

Copy link
Member

@LukaszRozmej LukaszRozmej left a 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?

@benaadams
Copy link
Member

We found lately with @obasekiosa that acquiring lock in LRU can take quite some time in very congested scenarios.

I assume shouldn't be that contended here?

@LukaszRozmej
Copy link
Member

LukaszRozmej commented Mar 27, 2024

We found lately with @obasekiosa that acquiring lock in LRU can take quite some time in very congested scenarios.

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();
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

Copy link
Contributor Author

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.

@asdacap asdacap merged commit 7059b45 into master Mar 28, 2024
67 checks passed
@asdacap asdacap deleted the perf/dont-redownload-downloaded-code branch March 28, 2024 15:01
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.

3 participants