-
Notifications
You must be signed in to change notification settings - Fork 7
Reduce states writes #937
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
base: master
Are you sure you want to change the base?
Reduce states writes #937
Conversation
🧪 Network TestsTo run network tests for this PR, use: gh workflow run network-tests.yml -f pr_number=937Available test options:
Test types: Results will be posted as workflow runs in the Actions tab. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #937 +/- ##
==========================================
- Coverage 46.28% 46.27% -0.02%
==========================================
Files 335 335
Lines 60474 60623 +149
Branches 60474 60623 +149
==========================================
+ Hits 27990 28051 +61
- Misses 31042 31104 +62
- Partials 1442 1468 +26 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
e49c772 to
6eb936e
Compare
3687b3f to
79a1a97
Compare
79a1a97 to
159efed
Compare
| // if it is finished, then we can just reload prev state | ||
| if last_task.store_new_state_task.is_finished() { | ||
| if last_task.store_new_state_task.is_finished() | ||
| && last_task.block_id.seqno.is_multiple_of(store_state_step) |
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.
Maybe it will be better to use more clear helper, e.g. self.state_node_adapter.check_should_store_state(..block_id.seqno)?
| while let Some((block_id, merkle_update)) = chain.pop() { | ||
| let prev_block_id = *state.block_id(); | ||
| state = rayon_run({ | ||
| move || state.par_make_next_state(&block_id, &merkle_update, Some(5)) |
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.
We already have 2+ places with a magic constant 5. Maybe it is worth to add a new param to the node config?
|
|
||
| tracing::debug!(target: tracing_targets::STATE_NODE_ADAPTER, "Load state: {}", block_id.as_short_id()); | ||
|
|
||
| let state = self |
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.
Now we have duplicating code here and in ShardStateStorage. Maybe it will be better to wrap the shard blocks cache into a some kind of ShardBlocksProvider that we can pass to ShardStateStorage::load_state(). So it will try to get shard block from provider or then load it from the db. Out of StateNodeAdapter we will pass an empty provider.
Pull Request Checklist
NODE CONFIGURATION MODEL CHANGES
Yes
Added .core_storage.store_shard_state_step: u32 with default value 2.
BLOCKCHAIN CONFIGURATION MODEL CHANGES
None
COMPATIBILITY
Fully compatible
SPECIAL DEPLOYMENT ACTIONS
Not Required
PERFORMANCE IMPACT
States GC stopped lagging -> cells cache stopped growing -> network degradation fixed. TPS increased by 10-15%. Storage timings a little decreased.
TESTS
Yes
Manual Tests
Manual Tests
Deploy 10M accounts then run 20k transfers