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

feat: stabilize flat storage for reads #8761

Merged
merged 11 commits into from
Apr 21, 2023
Merged

Conversation

Longarithm
Copy link
Member

@Longarithm Longarithm commented Mar 20, 2023

The code is literally removing protocol_feature_flat_state and moving feature to stable protocol. We also disable test_state_sync as this is part of refactor we can do in Q2.

Feature to stabilize

Here we stabilize Flat Storage for reads, which means that all state reads in the client during block processing will query flat storage instead of Trie. Flat Storage is another index for blockchain state, reducing number of DB accesses for state read from 2 * key.len() in the worst case to 2.

This will trigger background creation of flat storage, using 8 threads and finishing in 15h for RPC node and 2d for archival node. After that all non-contract reads will go through flat storage, for which special "chunk views" will be created. When protocol upgrade happens, contracts reads will go through flat storage as well. Also compute costs will change as Option 3 suggests here. It is to be merged separately, but we need to ensure that both costs change and flat storage go into next release together.

Context

Find more details in:

Testing and QA

Checklist

@Longarithm Longarithm self-assigned this Mar 23, 2023
@Longarithm Longarithm added the A-storage Area: storage and databases label Mar 23, 2023
@Longarithm Longarithm marked this pull request as ready for review March 25, 2023 00:36
@Longarithm Longarithm requested a review from a team as a code owner March 25, 2023 00:36
@Longarithm
Copy link
Member Author

Requesting approval, to merge somewhere before 1.34 cut.
@jakmeier I think the perfect time to merge it is just after setting up compute costs for protocol version 60.

Copy link
Contributor

@jakmeier jakmeier left a comment

Choose a reason for hiding this comment

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

Looks good on the surface. That is, the stabilization PR looks good to me but the decision to stabilize depends more on whether we think the rest of the code is ready. I believe we have reached that point but please @pugachAG confirm or deny.

In anycase, I will approve now but assume we still wait with the merge, as currently also noted in the PR. Merging right after compute costs are merged as suggested makes sense to me.

@Longarithm Longarithm changed the title feat, do not merge: stabilize flat storage for reads feat: stabilize flat storage for reads Apr 21, 2023
@near-bulldozer near-bulldozer bot merged commit e583d43 into near:master Apr 21, 2023
near-bulldozer bot pushed a commit that referenced this pull request Apr 24, 2023
There was a minor conflict while merging #8681 and #8761. Removing cfg(feature) because feature is stabilized now.
nikurt pushed a commit that referenced this pull request Apr 25, 2023
The code is literally removing `protocol_feature_flat_state` and moving feature to stable protocol. We also disable `test_state_sync` as this is part of refactor we can do in Q2.

## Feature to stabilize

Here we stabilize Flat Storage for reads, which means that all state reads in the client during block processing will query flat storage instead of Trie. Flat Storage is another index for blockchain state, reducing number of DB accesses for state read from `2 * key.len()` in the worst case to 2.

This will trigger background creation of flat storage, using 8 threads and finishing in 15h for RPC node and 2d for archival node. After that all non-contract reads will go through flat storage, for which special "chunk views" will be created. When protocol upgrade happens, contracts reads will go through flat storage as well. Also compute costs will change as Option 3 suggests [here](#8006 (comment)). It is to be merged separately, but we need to ensure that both costs change and flat storage go into next release together.

## Context

Find more details in:
- Overview: https://near.github.io/nearcore/architecture/storage/flat_storage.html
- Approved NEP: https://github.com/near/NEPs/blob/master/neps/nep-0339.md
- Tracking issue: #7327

## Testing and QA

* Flat storage structs are covered by unit tests;
* Integration tests check that chain behaviour is preserved and costs are changed as expected;
* Flat storage spent ~2 months in betanet with assertion that flat and trie `ValueRef`s are the same;
* We were running testnet/mainnet nodes for ~2 months with the same assertion. We checked that performance is not degraded, see e.g. https://nearinc.grafana.net/d/Vg9SREA4k/flat-storage-test?orgId=1&var-chain_id=mainnet&var-node_id=logunov-mainnet-fs-1&from=1677804289279&to=1678088806154 checking that even with finality lag of 50 blocks performance is not impacted. Small exception is that we updated data layout several times during development, but we checked that results are unchanged.

## Checklist
- [x] Include compute costs after they are merged - #8924
- [x] https://nayduck.near.org/#/run/2916
- [x] Update CHANGELOG.md to include this protocol feature in the `Unreleased` section.
nikurt pushed a commit that referenced this pull request Apr 25, 2023
There was a minor conflict while merging #8681 and #8761. Removing cfg(feature) because feature is stabilized now.
nikurt pushed a commit that referenced this pull request Apr 25, 2023
The code is literally removing `protocol_feature_flat_state` and moving feature to stable protocol. We also disable `test_state_sync` as this is part of refactor we can do in Q2.

## Feature to stabilize

Here we stabilize Flat Storage for reads, which means that all state reads in the client during block processing will query flat storage instead of Trie. Flat Storage is another index for blockchain state, reducing number of DB accesses for state read from `2 * key.len()` in the worst case to 2.

This will trigger background creation of flat storage, using 8 threads and finishing in 15h for RPC node and 2d for archival node. After that all non-contract reads will go through flat storage, for which special "chunk views" will be created. When protocol upgrade happens, contracts reads will go through flat storage as well. Also compute costs will change as Option 3 suggests [here](#8006 (comment)). It is to be merged separately, but we need to ensure that both costs change and flat storage go into next release together.

## Context

Find more details in:
- Overview: https://near.github.io/nearcore/architecture/storage/flat_storage.html
- Approved NEP: https://github.com/near/NEPs/blob/master/neps/nep-0339.md
- Tracking issue: #7327

## Testing and QA

* Flat storage structs are covered by unit tests;
* Integration tests check that chain behaviour is preserved and costs are changed as expected;
* Flat storage spent ~2 months in betanet with assertion that flat and trie `ValueRef`s are the same;
* We were running testnet/mainnet nodes for ~2 months with the same assertion. We checked that performance is not degraded, see e.g. https://nearinc.grafana.net/d/Vg9SREA4k/flat-storage-test?orgId=1&var-chain_id=mainnet&var-node_id=logunov-mainnet-fs-1&from=1677804289279&to=1678088806154 checking that even with finality lag of 50 blocks performance is not impacted. Small exception is that we updated data layout several times during development, but we checked that results are unchanged.

## Checklist
- [x] Include compute costs after they are merged - #8924
- [x] https://nayduck.near.org/#/run/2916
- [x] Update CHANGELOG.md to include this protocol feature in the `Unreleased` section.
nikurt pushed a commit that referenced this pull request Apr 28, 2023
The code is literally removing `protocol_feature_flat_state` and moving feature to stable protocol. We also disable `test_state_sync` as this is part of refactor we can do in Q2.

## Feature to stabilize

Here we stabilize Flat Storage for reads, which means that all state reads in the client during block processing will query flat storage instead of Trie. Flat Storage is another index for blockchain state, reducing number of DB accesses for state read from `2 * key.len()` in the worst case to 2.

This will trigger background creation of flat storage, using 8 threads and finishing in 15h for RPC node and 2d for archival node. After that all non-contract reads will go through flat storage, for which special "chunk views" will be created. When protocol upgrade happens, contracts reads will go through flat storage as well. Also compute costs will change as Option 3 suggests [here](#8006 (comment)). It is to be merged separately, but we need to ensure that both costs change and flat storage go into next release together.

## Context

Find more details in:
- Overview: https://near.github.io/nearcore/architecture/storage/flat_storage.html
- Approved NEP: https://github.com/near/NEPs/blob/master/neps/nep-0339.md
- Tracking issue: #7327

## Testing and QA

* Flat storage structs are covered by unit tests;
* Integration tests check that chain behaviour is preserved and costs are changed as expected;
* Flat storage spent ~2 months in betanet with assertion that flat and trie `ValueRef`s are the same;
* We were running testnet/mainnet nodes for ~2 months with the same assertion. We checked that performance is not degraded, see e.g. https://nearinc.grafana.net/d/Vg9SREA4k/flat-storage-test?orgId=1&var-chain_id=mainnet&var-node_id=logunov-mainnet-fs-1&from=1677804289279&to=1678088806154 checking that even with finality lag of 50 blocks performance is not impacted. Small exception is that we updated data layout several times during development, but we checked that results are unchanged.

## Checklist
- [x] Include compute costs after they are merged - #8924
- [x] https://nayduck.near.org/#/run/2916
- [x] Update CHANGELOG.md to include this protocol feature in the `Unreleased` section.
nikurt pushed a commit that referenced this pull request Apr 28, 2023
There was a minor conflict while merging #8681 and #8761. Removing cfg(feature) because feature is stabilized now.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-storage Area: storage and databases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants