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

allocations optim #626

Merged
merged 20 commits into from Jul 11, 2022
Merged

allocations optim #626

merged 20 commits into from Jul 11, 2022

Conversation

ETeissonniere
Copy link
Member

@ETeissonniere ETeissonniere commented Jun 24, 2022

Attempt to optimize the allocations pallet:

  • save 1 write by checking our issuance against our hardcoded maximum supply instead of tracking coins issued over time.
  • save 1 read by removing the emergency shutdown feature which wasn't used and could be replaced by the removal of the instant oracle.
  • save some non-quantifiable resources and indexing work for Subscan by removing our superfluous event at the end. We now rely on the two Transfer events from the transfer calls for indexing in both Subscan and the Nodle App.
  • saves hundreds of reads and writes by using a properly optimized batching extrinsic appropriately named batch which will allow us to be much more efficient in batching allocations together VS our current approach of using utility.batch_all. Deprecate allocate accordingly.

When reviewing, pay attention to:

  • the issuance checks, we do not want to have an in-prod problem because I messed up my computation here!
  • what other smart optimizations can we do without modifying the pallet's behavior? (as in, we still need our two transfer calls at the end for Subscan to pick allocations up)

@codecov
Copy link

codecov bot commented Jun 24, 2022

Codecov Report

Merging #626 (44aa064) into master (ee44942) will decrease coverage by 0.00%.
The diff coverage is 94.20%.

@@            Coverage Diff             @@
##           master     #626      +/-   ##
==========================================
- Coverage   89.42%   89.41%   -0.01%     
==========================================
  Files          37       34       -3     
  Lines        6229     6234       +5     
==========================================
+ Hits         5570     5574       +4     
- Misses        659      660       +1     
Impacted Files Coverage Δ
node/src/chain_spec.rs 0.00% <ø> (ø)
runtimes/eden/src/lib.rs 4.39% <ø> (+0.09%) ⬆️
runtimes/eden/src/pallets_nodle.rs 0.00% <ø> (ø)
runtimes/eden/src/version.rs 0.00% <ø> (ø)
pallets/allocations/src/lib.rs 82.69% <78.12%> (-3.98%) ⬇️
pallets/allocations/src/benchmarking.rs 94.73% <93.75%> (-5.27%) ⬇️
pallets/allocations/src/tests.rs 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ee44942...44aa064. Read the comment docs.

aliXsed
aliXsed previously approved these changes Jul 7, 2022
Copy link
Contributor

@aliXsed aliXsed left a comment

Choose a reason for hiding this comment

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

LGTM and multiple times more efficient than our current implementation, So it's a go from me.

pallet-emergency-shutdown = { default-features = false, path = "../emergency-shutdown" }
scale-info = { version = "2.0.1", default-features = false, features = [
"derive",
] }
Copy link
Collaborator

Choose a reason for hiding this comment

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

only formatting.

codec = { package = "parity-scale-codec", version = "3.0.0", default-features = false, features = ["derive"] }
codec = { package = "parity-scale-codec", version = "3.0.0", default-features = false, features = [
"derive",
] }
Copy link
Collaborator

Choose a reason for hiding this comment

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

only formatting

support = { path = "../../support" }

[dev-dependencies]
sp-core = { git = "https://github.com/paritytech/substrate", default-features = false , branch = "polkadot-v0.9.20" }
sp-core = { git = "https://github.com/paritytech/substrate", default-features = false, branch = "polkadot-v0.9.20" }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like only pallet-emergency-shutdown = { default-features = false, path = "../emergency-shutdown" } was removed but there are lots of minor changes which hide this.

ret.push((account, T::ExistentialDeposit::get() * 10u32.into()));
}

ret.try_into().unwrap()
Copy link
Collaborator

Choose a reason for hiding this comment

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

The unwrap can be avoided with something like this:

type BalanceOf<T> = <<T as Config>::Currency as Currency<<T as frame_system::Config>::AccountId>>::Balance;
fn make_batch<T: Config>(b: u32) -> BoundedVec<(T::AccountId, BalanceOf<T>), T::MaxAllocs> {
	let mut ret = BoundedVec::with_bounded_capacity(b as usize);

	for i in 0..b {
		let account = account("grantee", i, SEED);
		let _ = ret.try_push((account, T::ExistentialDeposit::get() * 10u32.into()));
	}
	ret
}

}: _(RawOrigin::Signed(config.oracle.clone()), config.grantee.clone(), T::ExistentialDeposit::get() * 10u32.into(), vec![])

batch {
let b in 1..T::MaxAllocs::get();
Copy link
Collaborator

Choose a reason for hiding this comment

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

When a range starts at 1 and does not include the last element it looks suspicious, is this intended here?

Copy link
Member Author

Choose a reason for hiding this comment

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

actually a batch len of 0, we'd throw an error since there are no allocations to perform

Copy link
Collaborator

@simonsso simonsso Jul 11, 2022

Choose a reason for hiding this comment

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

And the last to call should be T::MaxAllocs::get() or T::MaxAllocs::get()-1 ?

pallets/allocations/src/lib.rs Outdated Show resolved Hide resolved
@ETeissonniere ETeissonniere merged commit bc93fd8 into master Jul 11, 2022
@ETeissonniere ETeissonniere deleted the eliott/allocations-optim branch July 11, 2022 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants