Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1,539 changes: 650 additions & 889 deletions Cargo.lock

Large diffs are not rendered by default.

11 changes: 10 additions & 1 deletion client/allocator/src/freeing_bump.rs
Original file line number Diff line number Diff line change
Expand Up @@ -412,9 +412,18 @@ impl FreeingBumpHeapAllocator {

// Write the order in the occupied header.
Header::Occupied(order).write_into(mem, header_ptr)?;

self.total_size += order.size() + HEADER_SIZE;

if order.size() + HEADER_SIZE > 8 * 1024 * 1024 {
log::warn!(
target: LOG_TARGET,
"large allocation of {} detected, after allocation, total_size = {}, bumper = {}.",
order.size() + HEADER_SIZE,
self.total_size,
self.bumper,
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think this will definitely be spammy so it shouldn't stay as a warn.

This reminds me though, it'd probably be useful to add some sort of logging capability so that a log of every allocation from within the runtime could be dumped into a file; something like my Bytehound, but for the runtime instead of the native code. (Obviously totally out of scope of this PR though.)

Copy link
Contributor

Choose a reason for hiding this comment

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


log::trace!(
target: LOG_TARGET,
"after allocation, total_size = {}, bumper = {}.",
Expand Down
38 changes: 37 additions & 1 deletion frame/election-provider-multi-phase/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -948,6 +948,11 @@ pub mod pallet {
compute: ElectionCompute::Emergency,
};

Self::deposit_event(Event::SolutionStored {
election_compute: ElectionCompute::Emergency,
prev_ejected: Self::queued_solution().is_some(),
});

<QueuedSolution<T>>::put(solution);
Ok(())
}
Expand Down Expand Up @@ -1057,6 +1062,11 @@ pub mod pallet {
compute: ElectionCompute::Fallback,
};

Self::deposit_event(Event::SolutionStored {
election_compute: ElectionCompute::Fallback,
prev_ejected: Self::queued_solution().is_some(),
});

<QueuedSolution<T>>::put(solution);
Ok(())
}
Expand Down Expand Up @@ -1106,6 +1116,10 @@ pub mod pallet {
OcwCallWrongEra,
/// Snapshot metadata should exist but didn't.
MissingSnapshotMetadata,
/// Snapshot should exist but didn't.
MissingSnapshot,
/// Codec error.
Codec,
/// `Self::insert_submission` returned an invalid index.
InvalidSubmissionIndex,
/// The call is not allowed at this point.
Expand Down Expand Up @@ -1444,7 +1458,8 @@ impl<T: Config> Pallet<T> {

// Read the entire snapshot.
let RoundSnapshot { voters: snapshot_voters, targets: snapshot_targets } =
Self::snapshot().ok_or(FeasibilityError::SnapshotUnavailable)?;
Self::read_snapshot_with_preallocate()
.map_err(|_| FeasibilityError::SnapshotUnavailable)?;

// ----- Start building. First, we need some closures.
let cache = helpers::generate_voter_cache::<T::MinerConfig>(&snapshot_voters);
Expand Down Expand Up @@ -1495,6 +1510,27 @@ impl<T: Config> Pallet<T> {
Ok(ReadySolution { supports, compute, score })
}

/// Should be used instead of the getter of `Snapshot` in memory-bounded code paths.
fn read_snapshot_with_preallocate() -> Result<RoundSnapshot<T>, Error<T>> {
use codec::MaxEncodedLen;
let snap = Self::snapshot_metadata().ok_or(Error::<T>::MissingSnapshot)?;
let voters_size = snap.voters as usize * <VoterOf<T>>::max_encoded_len();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can probably be simplified to simply RoundSnapshot::max_encoded_len

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also, I wonder if what I did here can be applied to all storage items that are MaxEncodedLen?

let targets_size = snap.targets as usize * T::AccountId::max_encoded_len();

// we want to decode two vecs, which need two u32s at most for their size.
let initial_capacity = voters_size + targets_size + 4 + 4;
let mut buffer = Vec::<u8>::with_capacity(initial_capacity);

// fill this whole buffer, and decode into it.
buffer.resize(buffer.capacity(), 0);
let _leftover = sp_io::storage::read(&<Snapshot<T>>::hashed_key(), &mut buffer, 0)
.ok_or(Error::<T>::MissingSnapshot)?;

// buffer should have not re-allocated
debug_assert!(buffer.capacity() == initial_capacity);
<RoundSnapshot<T> as codec::Decode>::decode(&mut &*buffer).map_err(|_| Error::<T>::Codec)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm... so you manually calculate how much the scale-encoded value is supposed to take, preallocate a buffer, fetch it into that buffer and then decode it. Can you explain why is this necessary? How is this different than calling Snapshot::get()?

AFAIK this should already be done on the FFI boundary automatically, unless I'm missing something here? (And if I'm missing something then it'd be better to fix this on the FFI boundary so that it's fixed everywhere instead of manually fixing in on a case-by-case basis like here.)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, the FFI should also allocate buffer only to the actual size it has in the storage. Meaning it should even take less memory than you are calculating here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can provide some proof using the log that I added in the allocator, but I am pretty sure I consume less memory this way than calling Snapshot::get().

Copy link
Contributor

Choose a reason for hiding this comment

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

I can provide some proof using the log that I added in the allocator, but I am pretty sure I consume less memory this way than calling Snapshot::get().

This is how get works right now AFAIK:

  1. The host function to grab the value from storage gets triggered.
  2. The host reads the value from the storage into a host-side Vec.
  3. The host calls into the runtime's allocator and directly allocates a new chunk of memory accessible within the runtime.
  4. The host copies the data from the host-side Vec into the chunk of memory allocated at step (3).
  5. The host packs the pointer to that chunk of runtime memory along with its size into a single u64 value and returns that from the host function. (This is zero-cost as primitive types like u64 are natively supported by the FFI.)
  6. The runtime grabs that u64 value, unpacks it into a (ptr, size) pair and does a Vec::from_raw_parts to create a Vec<u8>.
  7. The runtime calls Decode::decode on that Vec<u8>.

So as you can see this shouldn't consume any extra memory since the host essentially directly allocates the runtime-side Vec into which it copies the data. It always allocates only the exact amount of bytes necessary, and directly copies the data without any extra resizes.

So either this code doesn't actually do anything different, or it does and the FFI layer doesn't work exactly as we think it does (in which case we should fix it).

So yeah, it'd be nice if you could double check that this actually does anything different.

Copy link
Member

Choose a reason for hiding this comment

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

@koute your description reads exactly on how the implementation is/should be.


/// Perform the tasks to be done after a new `elect` has been triggered:
///
/// 1. Increment round.
Expand Down
63 changes: 36 additions & 27 deletions frame/election-provider-multi-phase/src/unsigned.rs
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ impl<T: Config> Pallet<T> {
pub fn mine_solution(
) -> Result<(RawSolution<SolutionOf<T::MinerConfig>>, SolutionOrSnapshotSize), MinerError> {
let RoundSnapshot { voters, targets } =
Self::snapshot().ok_or(MinerError::SnapshotUnAvailable)?;
Self::read_snapshot_with_preallocate().map_err(|_| MinerError::SnapshotUnAvailable)?;
let desired_targets = Self::desired_targets().ok_or(MinerError::SnapshotUnAvailable)?;
let (solution, score, size) = Miner::<T::MinerConfig>::mine_solution_with_snapshot::<
T::Solver,
Expand All @@ -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 All @@ -463,7 +445,7 @@ impl<T: MinerConfig> Miner<T> {
SolutionOf::<T>::try_from(assignments).map(|s| s.encoded_size())
};

let ElectionResult { assignments, winners: _ } = election_result;
let ElectionResult { assignments, winners: _w } = election_result;

// Reduce (requires round-trip to staked form)
let sorted_assignments = {
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
2 changes: 1 addition & 1 deletion 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
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/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 @@ -710,7 +710,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