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

Allow EVM state to be flushed to disk every Nth block instead of every block #1532

Merged
merged 89 commits into from
Jan 28, 2020

Conversation

pathornteng
Copy link
Contributor

@pathornteng pathornteng commented Oct 9, 2019

Currently, we flush Patricia trie every tx. This commit operation is really expensive and it causes a lot of IOPS. Furthermore, once the trie is committed to disk, web3/truffle normally queries the newly committed data so the app loads the recently committed trie from disk shortly after.

This PR makes commit more efficient by holding multiple tries in memory and only flushing them every N blocks. This should significantly reduce the disk usage (less trie versions) and also IOPS (recent tries cached in memory)

Dependency: loomnetwork/go-ethereum#9

  • I added unit tests for any code that added
  • I updated the CHANGELOG.md
  • All IP is original and not copied from another source
  • I assign all copyright to Loom Network for the code in the pull request

app.go Outdated Show resolved Hide resolved
app.go Outdated Show resolved Hide resolved
app.go Outdated Show resolved Hide resolved
@pathornteng
Copy link
Contributor Author

pathornteng commented Nov 4, 2019

According to my own performance test, this PR

  1. Reduce the growth rate up tp 50%
  2. Reduce processing time for block replay up to 45%

store/evmstore.go Outdated Show resolved Hide resolved
rpc/query_server.go Outdated Show resolved Hide resolved
Due to the flush interval in the EvmStore some EVM state roots may never
be written to disk, so it's important to check that the root of the
state returned by EVMState.GetSnapshot() at a particular height matches
the EVM state root stored in the app store at that height, otherwise the
EVM state and app state returned by Application.ReadOnlyState() may end
up being out of sync.
The tests are supposed to test how the vm-prefixed keys are handled,
replacing the prefix makes the tests fairly pointless, so the prefix
has been restored.

A couple of the snapshot tests have been removed because the EvmStore
no longer caches writes until Commit, it now writes them out immediately.
The EVM root in the VersionedCachingStore cache wasn't being updated in
VersionedCachingStore.SaveVersion() which meant that the
snapshots obtained from the VersionedCachingStore contained a stale EVM
root that didn't match the EVM state in the snapshot.

This hack updates the cached EVM root in
VersionedCachingStore.SaveVersion() but it's not a proper fix for the
issue, the VersionedCachingStore shouldn't be aware of the EVM root at
all. The most sensible thing to do is probably to eliminate the
MultiWriterAppStore itself and update Application.Commit() to call
EvmStore.Commit(), and then store the EVM root in the
VersionedCachingStore.
@pathornteng
Copy link
Contributor Author

rebuild sesame

This setting works similarly to AppStore.IAVLFlushInterval.
@enlight
Copy link
Contributor

enlight commented Dec 12, 2019

Previously the EVM accessed data like so EVM -> VersionedCachingStore -> MultiWriterAppStore -> EvmStore -> evm.db, this PR changes it to EVM -> EVMState -> EvmStore -> evm.db, which means that the VersionedCachingStore is completely bypassed for EVM reads & writes, leaving only LevelDB caching.

Newer versions of go-ethereum (https://github.com/ethereum/go-ethereum/releases/tag/v1.8.19 or later) have added a cache to the trie.Database (ethereum/go-ethereum#18087) created by the EvmStore, this trie cache should compensate for the loss of VersionedCachingStore so we should update our go-ethereum fork.

https://github.com/ethereum/go-ethereum/releases/tag/v1.8.19 is the release in which the trie cache was initially added, it was optimized in https://github.com/ethereum/go-ethereum/releases/tag/v1.9.0 (ethereum/go-ethereum#19307), and then again in https://github.com/ethereum/go-ethereum/releases/tag/v1.9.8 (ethereum/go-ethereum#19971). Need to figure out if we can just bump our go-ethereum to v1.9.8.

@enlight
Copy link
Contributor

enlight commented Dec 13, 2019

There's also ethereum/go-ethereum#19953 (and a follow up PR ethereum/go-ethereum#20113 to fix some issues introduced by that PR) that was merged in go-ethereum v1.9.4 that may impact the trie.Database performance, will need to review it to make sure our usage of trie.Database is compatible with the changes in that PR.

@enlight
Copy link
Contributor

enlight commented Dec 16, 2019

In a follow up PR:

@enlight
Copy link
Contributor

enlight commented Dec 17, 2019

Previously the EVM accessed data like so EVM -> VersionedCachingStore -> MultiWriterAppStore -> EvmStore -> evm.db, this PR changes it to EVM -> EVMState -> EvmStore -> evm.db, which means that the VersionedCachingStore is completely bypassed for EVM reads & writes, leaving only LevelDB caching.

Actually this data access flow is only accurate when considering queries. Tx handlers never read from VersionedCachingStore, they only write to it, which means that the loss of the VersionedCachingStore shouldn't affect performance of validator and sentry nodes that aren't queried often... in fact we probably should've disabled the VersionedCachingStore by default on all those nodes a long time ago to save a bit of CPU & RAM.

@enlight enlight changed the title [WIP] Keep Patricia tries in memory and only commit every N blocks Allow EVM state to be flushed to disk on every Nth block instead of every block Dec 17, 2019
@enlight enlight changed the title Allow EVM state to be flushed to disk on every Nth block instead of every block Allow EVM state to be flushed to disk every Nth block instead of every block Dec 17, 2019
Copy link
Contributor

@enlight enlight left a comment

Choose a reason for hiding this comment

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

LGTM

@enlight enlight merged commit a6fba1c into master Jan 28, 2020
enlight added a commit that referenced this pull request Jan 29, 2020
This is a follow up to PR #1532, cleans up a couple of issues left over
from that PR:
- Eliminate GetSnapshot() from all the stores, only GetSnapshotAt() is used now.
- Fix VersionedCachingStore.GetSnapshotAt() to be atomic.

Also cleaned up the tests a little bit so they test using real stores
instead of buggy mock stores.
enlight added a commit that referenced this pull request Jan 29, 2020
This is a follow up to PR #1532, cleans up a couple of issues left over
from that PR:
- Eliminate GetSnapshot() from all the stores, only GetSnapshotAt() is used now.
- Fix VersionedCachingStore.GetSnapshotAt() to be atomic.

Also cleaned up the tests a little bit so they test using real stores
instead of buggy mock stores.
enlight added a commit that referenced this pull request Jan 30, 2020
MultiWriterAppStore was used to migrate EVM state keys from app.db to
evm.db, since all clusters have now been migrated this functionality is
no longer necessary, and is in fact incompatible with the changes
introduced in PR #1532.

This set of changes cleans out most of the obsolete code from that store:
- Eliminate writes to the EvmStore from MultiWriterAppStore.
- Eliminate Get/Has/Set/Delete from EvmStore.
- Eliminate use of AppStore.SaveEVMStateToIAVL config setting.
enlight added a commit that referenced this pull request Jan 30, 2020
MultiWriterAppStore was used to migrate EVM state keys from app.db to
evm.db, since all clusters have now been migrated this functionality is
no longer necessary, and is in fact incompatible with the changes
introduced in PR #1532.

This set of changes cleans out most of the obsolete code from that store:
- Eliminate writes to the EvmStore from MultiWriterAppStore.
- Eliminate Get/Has/Set/Delete from EvmStore.
- Eliminate use of AppStore.SaveEVMStateToIAVL config setting.
@enlight enlight deleted the in-memory-evmstate branch February 27, 2020 07:46
enlight added a commit that referenced this pull request Mar 13, 2020
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.

2 participants