-
Notifications
You must be signed in to change notification settings - Fork 264
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
feat: ADR for api cleanup and async commit #790
Changes from all commits
93ec06c
d450239
dd04c23
20e2f22
404172f
15e1a03
c504909
fce9a24
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,93 @@ | ||
# ADR ADR-002: API Cleanup to Improve Commit Performance | ||
|
||
## Changelog | ||
|
||
* 2023-06-06: First draft | ||
|
||
## Status | ||
|
||
DRAFT | ||
|
||
## Abstract | ||
|
||
This ADR proposes a cleanup of the API to make more understandable and maintainable the codebase of `iavl`. | ||
|
||
There is a lot of legacy code in the SDK that is not used anymore and can be removed. See the [Discussion](https://github.com/cosmos/iavl/issues/737) for more details. | ||
|
||
There are some proposals for the speedup of the `Commit` by the async writes. See the [Discussion](https://github.com/cosmos/cosmos-sdk/issues/16173) for more details. | ||
|
||
## Context | ||
|
||
The current implementation of `iavl` suffers from performance issues due to synchronous writes during the `Commit` process. To address this, the proposed changes aim to finalize the current version in memory and introduce asynchronous writes in the background. | ||
|
||
It is also necessary to refactor the `Set` function to accept a batch object for the atomic commitments. | ||
|
||
Moreover, the existing architecture of `iavl` lacks modularity and code organization: | ||
|
||
* The boundary between `ImmutableTree` and `MutableTree` is not clear. | ||
* There are many public methods in `nodeDB`, making it less structured. | ||
|
||
## Decision | ||
|
||
To make the codebase more clear and improve `Commit` performance, we propose the following changes: | ||
|
||
### Batch Set | ||
|
||
* Refactor the `Set` function to accept a batch object. | ||
* Eliminate the usage of `dbm.Batch` in `nodeDB`. | ||
|
||
### Async Commit | ||
|
||
* Finalize the current version in memory during `Commit`. | ||
* Perform async writes in the background. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what are the side effects to this? |
||
|
||
### API Cleanup | ||
|
||
* Make `nodeDB` methods private and remove all unused methods. | ||
* Follow the naming convention of the `store` module. | ||
|
||
The exposed API of `iavl` will be as follows: | ||
|
||
```go | ||
MutableTree interface { | ||
tac0turtle marked this conversation as resolved.
Show resolved
Hide resolved
|
||
SaveChangeSet(cs *ChangeSet) ([]byte, error) // this is for batch set | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what is changeset here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. similar to batch, just wanted to re-use the ChangeSet which is already defined in the iavl |
||
WorkingHash() []byte | ||
Commit() ([]byte, int64, error) // SaveVersion -> Commit | ||
Close() error // this is to make sure the async write is finished when shutdown | ||
DeleteVersionsTo(version int64) error | ||
LoadVersionForOverwriting(targetVersion int64) error | ||
GetImmutableTree(version int64) (*ImmutableTree, error) | ||
} | ||
|
||
ImmutableTree interface { | ||
Has(key []byte) (bool, error) | ||
Get(key []byte) ([]byte, error) | ||
Hash() []byte | ||
Iterator(start, end []byte, ascending bool) (types.Iterator, error) | ||
VersionExists(version int64) bool | ||
GetVersionedProof(key []byte, version int64) ([]byte, *cmtprotocrypto.ProofOps, error) | ||
} | ||
tac0turtle marked this conversation as resolved.
Show resolved
Hide resolved
|
||
``` | ||
|
||
## Consequences | ||
|
||
We expect the proposed changes to improve the performance of Commit through async writes. Additionally, the codebase will become more organized and maintainable. | ||
|
||
### Backwards Compatibility | ||
|
||
While this ADR will break the external API of `iavl`, it will not affect the internal state of nodeDB. Compatibility measures and migration steps will be necessary during the API cleanup to handle the breaking changes. | ||
|
||
### Positive | ||
|
||
* Atomicity of the `Commitments`. | ||
* Improved Commit performance through async writes. | ||
* Increased flexibility and ease of modification and refactoring for iavl. | ||
|
||
### Negative | ||
|
||
* Async Commit may result in increased memory usage and increased code complexity. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i would add the added complexity in the code base. the asyncommit adds a layer of complexity There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do you want to introduce some pseudo-code within this adr? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it can be added to a con, no need for pseudo-code There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sorry, a little confusing |
||
|
||
## References | ||
|
||
* <https://github.com/cosmos/cosmos-sdk/issues/16173> | ||
* <https://github.com/cosmos/iavl/issues/737> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the external api to users? the below seems to focus on internal changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you think iavl needs the external API interface to clarify? I plan to keep the majority external API