Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Optimize offchain worker memory usage a bit. #11454

Merged
merged 10 commits into from
May 24, 2022
1 change: 0 additions & 1 deletion client/allocator/src/freeing_bump.rs
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,6 @@ impl Link {
/// | 0 | next element link |
/// +--------------+-------------------+
/// ```
///
/// ## Occupied header
/// ```ignore
/// 64 32 0
Expand Down
59 changes: 34 additions & 25 deletions frame/election-provider-multi-phase/src/unsigned.rs
Original file line number Diff line number Diff line change
Expand Up @@ -169,27 +169,6 @@ impl<T: Config> Pallet<T> {
Ok((RawSolution { solution, score, round }, size))
}

/// Convert a raw solution from [`sp_npos_elections::ElectionResult`] to [`RawSolution`], which
/// is ready to be submitted to the chain.
///
/// Will always reduce the solution as well.
pub fn prepare_election_result<Accuracy: PerThing128>(
election_result: ElectionResult<T::AccountId, Accuracy>,
) -> Result<(RawSolution<SolutionOf<T::MinerConfig>>, SolutionOrSnapshotSize), MinerError> {
let RoundSnapshot { voters, targets } =
Self::snapshot().ok_or(MinerError::SnapshotUnAvailable)?;
let desired_targets = Self::desired_targets().ok_or(MinerError::SnapshotUnAvailable)?;
let (solution, score, size) =
Miner::<T::MinerConfig>::prepare_election_result_with_snapshot(
election_result,
voters,
targets,
desired_targets,
)?;
let round = Self::round();
Ok((RawSolution { solution, score, round }, size))
}

/// Attempt to restore a solution from cache. Otherwise, compute it fresh. Either way, submit
/// if our call's score is greater than that of the cached solution.
pub fn restore_or_compute_then_maybe_submit() -> Result<(), MinerError> {
Expand Down Expand Up @@ -441,7 +420,10 @@ impl<T: MinerConfig> Miner<T> {
})
}

/// Same as [`Pallet::prepare_election_result`], but the input snapshot mut be given as inputs.
/// Convert a raw solution from [`sp_npos_elections::ElectionResult`] to [`RawSolution`], which
/// is ready to be submitted to the chain.
///
/// Will always reduce the solution as well.
pub fn prepare_election_result_with_snapshot<Accuracy: PerThing128>(
election_result: ElectionResult<T::AccountId, Accuracy>,
voters: Vec<(T::AccountId, VoteWeight, BoundedVec<T::AccountId, T::MaxVotesPerVoter>)>,
Expand Down Expand Up @@ -1118,7 +1100,19 @@ mod tests {
distribution: vec![(10, PerU16::one())],
}],
};
let (solution, witness) = MultiPhase::prepare_election_result(result).unwrap();

let RoundSnapshot { voters, targets } = MultiPhase::snapshot().unwrap();
let desired_targets = MultiPhase::desired_targets().unwrap();

let (raw, score, witness) =
Miner::<Runtime>::prepare_election_result_with_snapshot(
result,
voters.clone(),
targets.clone(),
Comment on lines +1110 to +1111
Copy link
Contributor

Choose a reason for hiding this comment

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

Are those clones necessary? Can't we change the prepare_election_result_with_snapshot to take &[T] instead of a Vec<T> for those? It doesn't seem to actually need those owned from what I can see?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in the production code prepare_election_result_with_snapshot is the last place that we use these values so IMO it is cleaner to consume everything. Clones are only needed in tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah okay, that's fine then.

desired_targets,
)
.unwrap();
let solution = RawSolution { solution: raw, score, round: MultiPhase::round() };
assert_ok!(MultiPhase::unsigned_pre_dispatch_checks(&solution));
assert_ok!(MultiPhase::submit_unsigned(
Origin::none(),
Expand All @@ -1139,7 +1133,14 @@ mod tests {
},
],
};
let (solution, _) = MultiPhase::prepare_election_result(result).unwrap();
let (raw, score, _) = Miner::<Runtime>::prepare_election_result_with_snapshot(
result,
voters.clone(),
targets.clone(),
desired_targets,
)
.unwrap();
let solution = RawSolution { solution: raw, score, round: MultiPhase::round() };
// 12 is not 50% more than 10
assert_eq!(solution.score.minimal_stake, 12);
assert_noop!(
Expand All @@ -1161,7 +1162,15 @@ mod tests {
},
],
};
let (solution, witness) = MultiPhase::prepare_election_result(result).unwrap();
let (raw, score, witness) =
Miner::<Runtime>::prepare_election_result_with_snapshot(
result,
voters.clone(),
targets.clone(),
desired_targets,
)
.unwrap();
let solution = RawSolution { solution: raw, score, round: MultiPhase::round() };
assert_eq!(solution.score.minimal_stake, 17);

// and it is fine
Expand Down
4 changes: 2 additions & 2 deletions frame/support/src/storage/types/nmap.rs
Original file line number Diff line number Diff line change
Expand Up @@ -544,7 +544,7 @@ mod test {
use crate::{
hash::{StorageHasher as _, *},
metadata::{StorageEntryModifier, StorageHasher},
storage::types::{Key, Key as NMapKey, ValueQuery},
storage::types::{Key, ValueQuery},
};
use sp_io::{hashing::twox_128, TestExternalities};

Expand Down Expand Up @@ -590,7 +590,7 @@ mod test {

{
#[crate::storage_alias]
type Foo = StorageNMap<test, (NMapKey<Blake2_128Concat, u16>), u32>;
type Foo = StorageNMap<test, (Key<Blake2_128Concat, u16>), u32>;

assert_eq!(Foo::contains_key((3,)), true);
assert_eq!(Foo::get((3,)), Some(10));
Expand Down
23 changes: 11 additions & 12 deletions primitives/npos-elections/src/assignments.rs
Original file line number Diff line number Diff line change
Expand Up @@ -120,18 +120,17 @@ impl<AccountId> StakedAssignment<AccountId> {
AccountId: IdentifierT,
{
let stake = self.total();
let distribution = self
.distribution
.into_iter()
.filter_map(|(target, w)| {
let per_thing = P::from_rational(w, stake);
if per_thing == Bounded::min_value() {
None
} else {
Some((target, per_thing))
}
})
.collect::<Vec<(AccountId, P)>>();
// most likely, the size of the staked assignment and normal assignments will be the same,
// so we pre-allocate it to prevent a sudden 2x allocation. `filter_map` starts with a size
Copy link
Member

Choose a reason for hiding this comment

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

the context about filter map makes less sense if you dont know the diff.

// of 0 by default.
// https://www.reddit.com/r/rust/comments/3spfh1/does_collect_allocate_more_than_once_while/
let mut distribution = Vec::<(AccountId, P)>::with_capacity(self.distribution.len());
self.distribution.into_iter().for_each(|(target, w)| {
let per_thing = P::from_rational(w, stake);
if per_thing != Bounded::min_value() {
distribution.push((target, per_thing));
}
});

Assignment { who: self.who, distribution }
}
Expand Down
5 changes: 5 additions & 0 deletions utils/frame/try-runtime/cli/src/commands/execute_block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,11 @@ where
.overwrite_online_at(parent_hash.to_owned());

let builder = if command.overwrite_wasm_code {
log::info!(
target: LOG_TARGET,
"replacing the in-storage :code: with the local code from {}'s chain_spec (your local repo)",
config.chain_spec.name(),
);
let (code_key, code) = extract_code(&config.chain_spec)?;
builder.inject_hashed_key_value(&[(code_key, code)])
} else {
Expand Down
5 changes: 5 additions & 0 deletions utils/frame/try-runtime/cli/src/commands/offchain_worker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,11 @@ where
let builder = command.state.builder::<Block>()?;

let builder = if command.overwrite_wasm_code {
log::info!(
target: LOG_TARGET,
"replacing the in-storage :code: with the local code from {}'s chain_spec (your local repo)",
config.chain_spec.name(),
);
let (code_key, code) = extract_code(&config.chain_spec)?;
builder.inject_hashed_key_value(&[(code_key, code)])
} else {
Expand Down
2 changes: 1 addition & 1 deletion utils/frame/try-runtime/cli/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -722,7 +722,7 @@ pub(crate) fn state_machine_call<Block: BlockT, D: NativeExecutionDispatch + 'st
sp_core::testing::TaskExecutor::new(),
)
.execute(execution.into())
.map_err(|e| format!("failed to execute 'TryRuntime_on_runtime_upgrade': {}", e))
.map_err(|e| format!("failed to execute '{}': {}", method, e))
.map_err::<sc_cli::Error, _>(Into::into)?;

Ok((changes, encoded_results))
Expand Down