Optimise memory usage while loading genesis #4269

posvyatokum · 2021-05-02T13:35:09Z

Adding option of streaming records to Genesis
Applying genesis records in chunks (not only in case of streaming)

posvyatokum · 2021-05-02T13:35:39Z

Probably should adjust the chunk size. With this patch, the node is starting in 15 minutes, without it -- in 8. As for memory, the new code consumes 3 GB (old code -- 14 GB), so there is room to go.

matklad

Left some Rust-related comments (although most of them are "oh, that's actually harder than I thought, I see now why you do this the way you did").

One thing I am worrying a lot is that now processing records might fail with an io::Error and we do not reflect that in the types. We just unwrap inside.I feel that's a significant change of geneissis contract, and that's its better to expose the fact we can fail at the call site. But I see how this will make this already quite a big change larger. I don't necessary want to block on this, but it does seems that silencing IO errors that way increases the technical debt substantionaly.

core/chain-configs/src/genesis_config.rs

runtime/runtime/src/lib.rs

matklad · 2021-05-03T12:51:27Z

runtime/runtime/src/lib.rs

+        let mut genesis_state_applier =
+            GenesisStateApplier::new(tries, shard_id, validators, config);
+        genesis_state_applier.apply(genesis, shard_account_ids)


Ok, at least it seems that this dosen't need to be a stateful object, as we apply all the stuff in one go anyway.

I'm not sure if I've done what you wanted, it still looks weird. Also this part of code moved to runtime/runtime/src/genesis.rs

core/chain-configs/src/genesis_config.rs

runtime/runtime/src/lib.rs

…atch

bowenwang1996

What is the peak memory usage when neard loads testnet genesis after this change?

bowenwang1996 · 2021-05-07T00:31:05Z

core/chain-configs/src/genesis_config.rs

        genesis.config.total_supply = get_initial_supply(&genesis.records.as_ref());
        genesis
    }

+    pub fn new_as_is(


I don't think this is the best name :)

posvyatokum · 2021-05-07T10:53:40Z

What is the peak memory usage when neard loads testnet genesis after this change?

5 GB

EgorKulikov · 2021-05-07T13:38:54Z

core/chain-configs/src/genesis_config.rs

+    }
+}
+
+fn stream_records_from_file(


Maybe this is logically static method of RecordsProcessor

(not really important): I'd say it's the other way around -- this function is the public API, and RecordsProcessor is it's private implementation detail. If it were a separate module (it doesn't have to be), it's best written as

// genesis_streaming.rs pub fn stream_records_from_file( reader: impl Read, mut callback: impl FnMut(StateRecord), ) -> serde_json::Result<()> { let mut deserializer = serde_json::Deserializer::from_reader(reader); let records_processor = RecordsProcessor { sink: &mut callback }; deserializer.deserialize_any(records_processor) } struct RecordsProcessor<F> { ... }

matklad

Left couple of comments, nothing blocking!

matklad · 2021-05-07T15:10:35Z

core/chain-configs/src/genesis_config.rs

+    fn stream_records_with_callback(
+        &self,
+        callback: impl FnMut(StateRecord),
+    ) -> std::io::Result<()> {


it's idiomatic to use std::io; and use io::Result and io::Error

matklad · 2021-05-07T15:10:38Z

core/chain-configs/src/genesis_config.rs

+
+    pub fn for_each_record(&self, mut callback: impl FnMut(&StateRecord)) {


Hm, I am still not feeling completely comfortable with just .expecting errors here. This signature looks as if it's a pure in-memory function that can't fail. I suggest adding a comment here explaining that this actually does disk IO and can panic if the genesis file is removed.

On the over hand, we already have validate_genesis which handles errors via panics, so in that sense this doesn't make the code worse...

matklad · 2021-05-07T15:14:11Z

core/chain-configs/src/genesis_config.rs

+    }
+}
+
+fn stream_records_from_file(


(not really important): I'd say it's the other way around -- this function is the public API, and RecordsProcessor is it's private implementation detail. If it were a separate module (it doesn't have to be), it's best written as

// genesis_streaming.rs pub fn stream_records_from_file( reader: impl Read, mut callback: impl FnMut(StateRecord), ) -> serde_json::Result<()> { let mut deserializer = serde_json::Deserializer::from_reader(reader); let records_processor = RecordsProcessor { sink: &mut callback }; deserializer.deserialize_any(records_processor) } struct RecordsProcessor<F> { ... }

matklad · 2021-05-07T15:17:44Z

core/chain-configs/src/genesis_config.rs

@@ -299,11 +375,16 @@ impl GenesisJsonHasher {

 impl Genesis {
    pub fn new(config: GenesisConfig, records: GenesisRecords) -> Self {
-        let mut genesis = Self { config, records, phantom: PhantomData };
+        let mut genesis =
+            Self { config, records, records_file: PathBuf::new(), phantom: PhantomData };


I wonder it this should be records_file: None? That is, use Option to relfect in the type system that we might not have records_file at all.

matklad · 2021-05-07T15:18:16Z

core/primitives/src/state_record.rs

+    match &state_record {
+        StateRecord::Account { account_id, .. }
+        | StateRecord::AccessKey { account_id, .. }
+        | StateRecord::Contract { account_id, .. }
+        | StateRecord::ReceivedData { account_id, .. }
+        | StateRecord::Data { account_id, .. } => account_id,
+        StateRecord::PostponedReceipt(receipt) | StateRecord::DelayedReceipt(receipt) => {
+            &receipt.receiver_id
+        }


Nice idiom!

matklad · 2021-05-07T15:18:50Z

core/primitives/src/state_record.rs

@@ -147,3 +147,16 @@ fn to_printable(blob: &[u8]) -> String {
        }
    }
 }
+
+pub fn state_record_to_account_id(state_record: &StateRecord) -> &AccountId {
+    match &state_record {


Suggested change

match &state_record {

match state_record {

state_record is already a ref

matklad · 2021-05-07T15:21:02Z

neard/src/genesis_validate.rs

+    pub fn new(genesis_config: &'a GenesisConfig) -> Self {
+        return Self {
+            genesis_config,
+            total_supply: 0,
+            staked_accounts: HashMap::new(),
+            account_ids: HashSet::new(),
+            access_key_account_ids: HashSet::new(),
+            contract_account_ids: HashSet::new(),
+        };
+    }
+
+    pub fn process_record(&mut self, record: &StateRecord) {


Suggested change

pub fn new(genesis_config: &'a GenesisConfig) -> Self {

return Self {

genesis_config,

total_supply: 0,

staked_accounts: HashMap::new(),

account_ids: HashSet::new(),

access_key_account_ids: HashSet::new(),

contract_account_ids: HashSet::new(),

};

}

pub fn process_record(&mut self, record: &StateRecord) {

fn new(genesis_config: &'a GenesisConfig) -> Self {

return Self {

genesis_config,

total_supply: 0,

staked_accounts: HashMap::new(),

account_ids: HashSet::new(),

access_key_account_ids: HashSet::new(),

contract_account_ids: HashSet::new(),

};

}

fn process_record(&mut self, record: &StateRecord) {

pub is not needed here. I also find it useful to not have unnecessary pubs. That way, when you see a pub, you know that its a cross-crate interface.

matklad · 2021-05-07T15:21:56Z

neard/src/genesis_validate.rs

+            .validators
+            .clone()
+            .into_iter()


Suggested change

.validators

.clone()

.into_iter()

.validators

.iter()

.cloned()

This will be a tiiiny bit better, as the vector itself won't be cloned

matklad · 2021-05-07T15:23:39Z

neard/src/genesis_validate.rs

+}
+
+/// Validate genesis config and records. Panics if genesis is ill-formed.
+pub fn validate_genesis(genesis: &Genesis) {


I don't think we have any existing conventions around this, but I personally prefer relatively strongly to keep module's public API at the top. That way, when you read a module, you can understand at a glance it's API: https://github.com/rust-analyzer/rust-analyzer/blob/master/docs/dev/style.md#order-of-items

matklad · 2021-05-07T15:26:10Z

runtime/runtime/src/lib.rs

@@ -2251,6 +2122,8 @@ mod tests {

    #[test]
    fn test_delete_key_add_key() {
+        use near_primitives::account::AccessKey;


This function is alerady inside the cfg(test) module, so you can put the import at the top of the module:

#[cfg(test)] mod tests { use super::*; use near_crypto::{InMemorySigner, KeyType, Signer}; use near_primitives::errors::ReceiptValidationError; use near_primitives::hash::hash; use near_primitives::profile::ProfileData; use near_primitives::test_utils::{account_new, MockEpochInfoProvider}; use near_primitives::transaction::{ AddKeyAction, DeleteKeyAction, FunctionCallAction, TransferAction, }; use near_primitives::types::MerkleHash; use near_primitives::version::PROTOCOL_VERSION; use near_primitives::account::AccessKey; // new import use near_store::test_utils::create_tries; use near_store::StoreCompiledContractCache; use std::sync::Arc; use testlib::runtime_utils::{alice_account, bob_account};

frol · 2021-08-02T13:13:53Z

@posvyatokum

Probably should adjust the chunk size. With this patch, the node is starting in 15 minutes, without it -- in 8. As for memory, the new code consumes 3 GB (old code -- 14 GB), so there is room to go.

I just started a fresh testnet node based on 1.20.0-rc.2 release (which includes this PR, I believe), and observed that it consumed 10GB of RAM on neard init. Is that expected?

posvyatokum added 11 commits May 1, 2021 23:30

opt out of records loading while loading config

d595fab

create GenesisValidator to allow validation while streaming records

a3c5cd3

adding streaming records from genesis and records files

fb76e11

adding records file path and streaming to Genesis

b3a61de

stream records from file in json_hash if records field is empty

46f6d7d

stream records from file in genesis validation

b1f9e62

encapsulate records iteration vs streaming logic

9db4ba3

apply genesis state in chunks

6408175

refactor process_records usage

f58dc4c

switch to streaming genesis in neard run

9ed31e5

remove unused import

e995761

posvyatokum requested review from matklad and bowenwang1996 May 2, 2021 13:35

posvyatokum requested review from EgorKulikov, evgenykuzyakov, frol, MaksymZavershynskyi and SkidanovAlex as code owners May 2, 2021 13:35

posvyatokum linked an issue May 2, 2021 that may be closed by this pull request

Optimize genesis loading speed and memory usage #4190

Closed

matklad reviewed May 3, 2021

View reviewed changes

bowenwang1996 reviewed May 3, 2021

View reviewed changes

core/chain-configs/src/genesis_config.rs Outdated Show resolved Hide resolved

runtime/runtime/src/lib.rs Outdated Show resolved Hide resolved

runtime/runtime/src/lib.rs Outdated Show resolved Hide resolved

EgorKulikov and others added 8 commits May 5, 2021 11:01

Merge branch 'master' into optimize-genesis-loading

cb1677c

increase chunk size

c64669d

punctuation

b546bf5

refactor genesis streaming

b26d7de

refactor ref

87d2f60

remove pub from GenesisValidator

455b786

Borrow<StateRecord> -> &StateRecord

0f5d445

process_records -> for_each_record

4f606ec

posvyatokum added 8 commits May 5, 2021 15:53

move genesis processing to separate file & genesis chunk -> genesis b…

3fe0888

…atch

make GenesisStateApplier stateless

9c4ac48

pass error from File::open

d63d621

make RecordsProcessor visitor universal

fb04fd7

Merge branch 'master' into optimize-genesis-loading

2597ee1

fix imports

b0ad7f3

move imports to tests

2959101

Merge branch 'master' into optimize-genesis-loading

4587d00

bowenwang1996 approved these changes May 7, 2021

View reviewed changes

new_as_is -> new_with_path

acfdf61

Merge branch 'master' into optimize-genesis-loading

fd2e67b

EgorKulikov reviewed May 7, 2021

View reviewed changes

matklad approved these changes May 7, 2021

View reviewed changes

minor fixes: imports and visibility

7c918ca

posvyatokum merged commit 884c6d1 into master May 7, 2021

posvyatokum deleted the optimize-genesis-loading branch May 7, 2021 18:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimise memory usage while loading genesis #4269

Optimise memory usage while loading genesis #4269

posvyatokum commented May 2, 2021

posvyatokum commented May 2, 2021

matklad left a comment

matklad May 3, 2021

posvyatokum May 5, 2021

bowenwang1996 left a comment

bowenwang1996 May 7, 2021

posvyatokum commented May 7, 2021

EgorKulikov May 7, 2021

matklad May 7, 2021

matklad left a comment

matklad May 7, 2021

matklad May 7, 2021

matklad May 7, 2021

matklad May 7, 2021

matklad May 7, 2021

matklad May 7, 2021

matklad May 7, 2021

matklad May 7, 2021

matklad May 7, 2021

matklad May 7, 2021

frol commented Aug 2, 2021


		pub fn for_each_record(&self, mut callback: impl FnMut(&StateRecord)) {

Optimise memory usage while loading genesis #4269

Optimise memory usage while loading genesis #4269

Conversation

posvyatokum commented May 2, 2021

posvyatokum commented May 2, 2021

matklad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bowenwang1996 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

posvyatokum commented May 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matklad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frol commented Aug 2, 2021