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

add ECPoint cache #2616

Merged
merged 8 commits into from
Nov 1, 2021
Merged

add ECPoint cache #2616

merged 8 commits into from
Nov 1, 2021

Conversation

Qiao-Jin
Copy link
Contributor

Close #2614

@roman-khimov
Copy link
Contributor

tps_single

p = DecompressPoint(yTilde, X1, curve);
p._compressedPoint = encoded.ToArray();
byte[] compressedPoint = encoded.ToArray();
if (pointCache.TryGet(compressedPoint, out ECPoint inventory))
Copy link
Member

Choose a reason for hiding this comment

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

The cache is not curve dependant

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have fixed

Jim8y
Jim8y previously approved these changes Oct 23, 2021
@Qiao-Jin
Copy link
Contributor Author

Ping

@erikzhang
Copy link
Member

Is this more effective for offline synchronization?

@Qiao-Jin
Copy link
Contributor Author

Qiao-Jin commented Oct 27, 2021

Is this more effective for offline synchronization?

Yes, without this PR it would need about 25 seconds to sync 10000 blocks offline in "--noverify" mode.
With this PR it would need about 4 seconds

@erikzhang
Copy link
Member

Then I suggest adding this cache to the offline synchronization module.

@Qiao-Jin
Copy link
Contributor Author

Then I suggest adding this cache to the offline synchronization module.

But according to #2616 (comment) this PR can also enhance TPS...

@roman-khimov
Copy link
Contributor

roman-khimov commented Oct 27, 2021

I'd just comment on our experience with this. Way back when in #1321 days we've added similar cache specifically for block processing, the difference there was huge, 30% faster block imports were really important for us. Then at some point in time we've started optimizing our node and discovered that key decoding is an important part of the process there also, so we've generalized this cache and made it system-wide (applicable for any key decoding), that had improved our TPS metrics somewhat (as a part of nspcc-dev/neo-go#1396, ~5% improvement from this particular change).

On C# side this PR easily (there is not a lot of code that it adds) improves TPS metrics by 14%. Sure, this particular neo-bench test is somewhat complimentary for this kind of change because it doesn't use many addresses and thus always has 100% cache hit for all keys. At the same time on a real network we're still very likely to see the same keys being used for transactions in subsequent blocks, so it's important and it will help there (I'd only suggest making the cache a bit larger, keys are rather small, so having a 1000 of them in memory is not a big problem).

@superboyiii
Copy link
Member

Then I suggest adding this cache to the offline synchronization module.

No ImportBlocks any more.

@Qiao-Jin
Copy link
Contributor Author

Qiao-Jin commented Nov 1, 2021

merge?

@superboyiii
Copy link
Member

@shargon @erikzhang Merge?

@superboyiii
Copy link
Member

The right one is with this PR, 2x speed on offline sync.
1635747774(1)

@erikzhang erikzhang merged commit 817ab62 into neo-project:master Nov 1, 2021
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.

Compressed ECPoint might severely reduce offline sync speed
7 participants