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

TaskManager optimization #2615

Merged
merged 10 commits into from
Nov 1, 2021

Conversation

Qiao-Jin
Copy link
Contributor

@Qiao-Jin Qiao-Jin commented Oct 19, 2021

An optimization upon taskmanager and corresponding logic. Fixes logic error in edge cases & reduces network wastes.

if (!header.Verify(system.Settings, snapshot, system.HeaderCache)) break;
if (!header.Verify(system.Settings, snapshot, system.HeaderCache))
{
Sender.Tell(VerifyResult.Invalid);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Sender.Tell(VerifyResult.Invalid);
Sender.Tell(VerifyResult.Invalid);
break;

Copy link
Member

Choose a reason for hiding this comment

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

Why send VerifyResult.Invalid to RemoteNode? It doesn't process the message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is similar to some other places where akka replies are sent even not processed

Copy link
Member

Choose a reason for hiding this comment

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

Please remove it. VerifyResult is not used for this purpose.

Co-authored-by: Shargon <shargon@gmail.com>
@Qiao-Jin
Copy link
Contributor Author

Ping

@vncoelho
Copy link
Member

I need 1 week to review more carefully, @Qiao-Jin.

@superboyiii
Copy link
Member

superboyiii commented Oct 28, 2021

Test PASS
I start both at the same time. And till 52000 blocks, the difference is very clear. The right one is from this PR and really performance much better.
1635403406(1)

superboyiii
superboyiii previously approved these changes Oct 28, 2021
@superboyiii
Copy link
Member

@shargon Need your review again.

@superboyiii
Copy link
Member

And to more than 130000, the difference will be much more huge.
1635403928(1)

shargon
shargon previously approved these changes Oct 28, 2021
@erikzhang erikzhang dismissed stale reviews from shargon and superboyiii via a522f65 October 28, 2021 08:05
@@ -236,7 +239,10 @@ private void OnTaskCompleted(IInventory inventory)
session.ReceivedBlock.Add(block.Index, block);
}
}
RequestTasks(Sender, session);
else
Copy link
Member

Choose a reason for hiding this comment

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

Why this else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. If RequestTask is called upon receiving a block, taskManager will seek this block again from its neighbor
  2. RequestTask will be called upon OnPersistCompleted for this block eventually

vncoelho
vncoelho previously approved these changes Oct 28, 2021
Copy link
Member

@vncoelho vncoelho left a comment

Choose a reason for hiding this comment

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

Tested on NeoCompiler. Nice move and good improvement for congestion.

@vncoelho vncoelho dismissed their stale review October 28, 2021 14:44

I will double check here the version I used.

Copy link
Member

@vncoelho vncoelho left a comment

Choose a reason for hiding this comment

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

Confirmed.

@Qiao-Jin
Copy link
Contributor Author

Qiao-Jin commented Nov 1, 2021

merge?

@superboyiii
Copy link
Member

@erikzhang @shargon Merge?

@erikzhang erikzhang merged commit ac95441 into neo-project:master Nov 1, 2021
@Qiao-Jin Qiao-Jin deleted the taskmanager_optimization branch November 4, 2021 06:44
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.

6 participants