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

Optimize OnPersist #3145

Open
shargon opened this issue Feb 17, 2024 · 14 comments · May be fixed by #3669
Open

Optimize OnPersist #3145

shargon opened this issue Feb 17, 2024 · 14 comments · May be fixed by #3669
Labels
Backlog Backlog issues and PRs Discussion Initial issue state - proposed but not yet accepted

Comments

@shargon
Copy link
Member

shargon commented Feb 17, 2024

Developing the rpc storage in test engine (neo-project/neo-devpack-dotnet#904) I have realized how "expensive" it is to calculate the gas per block.

Currently, we store a history of gas changes, which is necessary for calculating the reward of accounts when they call claim, but it is not necessary for the OnPersist of the block. However, we iterate backwards to calculate it in the same way during the block's persist, something completely unnecessary.

It can be optimized by avoiding iterating over the storage, we have 2 options.

Opinions?

@shargon shargon added the Discussion Initial issue state - proposed but not yet accepted label Feb 17, 2024
@shargon
Copy link
Member Author

shargon commented Feb 17, 2024

I vote for replace the Prefix_GasPerBlock

@roman-khimov
Copy link
Contributor

There is no need to change the DB scheme and store duplicating data, just cache it.
https://github.com/nspcc-dev/neo-go/blob/71fb759c0d009218437d0048f7ba085a2a74c66f/pkg/core/native/native_neo.go#L45-L47

@shargon
Copy link
Member Author

shargon commented Feb 17, 2024

There is no need to change the DB scheme and store duplicating data, just cache it. https://github.com/nspcc-dev/neo-go/blob/71fb759c0d009218437d0048f7ba085a2a74c66f/pkg/core/native/native_neo.go#L45-L47

That's solve the node issue, but it won't solve external readers like rpc storage.

@shargon
Copy link
Member Author

shargon commented Feb 17, 2024

@cschuchardt88
Copy link
Member

cschuchardt88 commented Feb 17, 2024

@shargon what is the Key parts for Prefix_GasPerBlock? KeyBuilder(id, Prefix_GasPerBlock).add(.....)? Also MemoryStore is really slow. That is where the problem, I'm pretty sure.

@shargon
Copy link
Member Author

shargon commented Feb 17, 2024

Currently, we store a history of gas changes, which is necessary for calculating the reward of accounts when they call claim, but it is not necessary for the OnPersist of the block. However, we iterate backwards to calculate it in the same way during the block's persist, something completely unnecessary.

@shargon
Copy link
Member Author

shargon commented Feb 17, 2024

@neo-project/core How to test? debug the node, and press F11 (step into) in GasPerBlock... you will see all the Find and Seek logic, this should be a direct read.

@shargon
Copy link
Member Author

shargon commented Feb 20, 2024

We can also optimize with the same technique GetDesignatedByRole

@shargon
Copy link
Member Author

shargon commented Feb 28, 2024

@neo-project/core @roman-khimov if we will need to resync the chain for 3.7, this is something to think about it, GasPerBlock and GetDesignatedByRole are used in all blocks.

@roman-khimov
Copy link
Contributor

This problem is still not worth changing the DB scheme, caching solves it completely and it'd be faster than additional data in the DB anyway.

@shargon
Copy link
Member Author

shargon commented Feb 28, 2024

This problem is still not worth changing the DB scheme, caching solves it completely and it'd be faster than additional data in the DB anyway.

And remove the nefFile from contractState? We only need the Script and tokens, and all the nefFile is deserialized in all contract calls, at least we can avoid the checksum check

@shargon shargon mentioned this issue Feb 28, 2024
1 task
@roman-khimov
Copy link
Contributor

This would remove the symmetry of deploy/getcontract. We deploy NEF/manifest, we can expect to get them back the same way. We have a number of external interfaces that return NEFs as well, this change can break them.

Deserialization optimization is OK. We actually have a contract cache in NeoGo, so we don't deserialize often.

@AnnaShaleva
Copy link
Member

AnnaShaleva commented Mar 3, 2024

Vote up for cache usage instead of storage scheme change.

That's solve the node issue, but it won't solve external readers like rpc storage.

Could you explain, why? Because in our implementation a similar cache is available for external users via exported method (e.g. external consensus service uses https://github.com/nspcc-dev/neo-go/blob/71fb759c0d009218437d0048f7ba085a2a74c66f/pkg/core/native/native_neo.go#L1120 without any issues). We initialize this cache when needed (on node resume or test historic VM creation basically, ref. https://github.com/nspcc-dev/neo-go/blob/71fb759c0d009218437d0048f7ba085a2a74c66f/pkg/core/native/native_neo.go#L329), and it doesn't cost a lot. The only caveat is that we should keep it up-to-date and relevant, but it's possible.

@cschuchardt88 cschuchardt88 added the Backlog Backlog issues and PRs label Sep 5, 2024
@shargon shargon linked a pull request Jan 13, 2025 that will close this issue
15 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backlog Backlog issues and PRs Discussion Initial issue state - proposed but not yet accepted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants