-
Notifications
You must be signed in to change notification settings - Fork 696
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
process_inherent_data: use an in memory overlay to minimize reads and writes to storage #3644
Conversation
also no need to sort by core index any more
…. optimise process_candidates to be O(N)
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
bot bench polkadot-pallet --runtime=westend --pallet=runtime_parachains::paras_inherent |
"$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=westend --target_dir=polkadot --pallet=runtime_parachains::paras_inherent was queued. Comment |
@sandreim Command
|
bot bench polkadot-pallet --runtime=westend --pallet=runtime_parachains::paras_inherent |
"$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=westend --target_dir=polkadot --pallet=runtime_parachains::paras_inherent was queued. Comment |
@sandreim Command
|
bot bench polkadot-pallet --runtime=westend --pallet=runtime_parachains::paras_inherent |
@ggwpez https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5517794 was started for your command Comment |
@ggwpez Command |
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
dd1e8f9
to
728aa29
Compare
bot bench polkadot-pallet --runtime=westend --pallet=runtime_parachains::paras_inherent |
@sandreim https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5520978 was started for your command Comment |
@sandreim Command |
The CI pipeline was cancelled due to failure one of the required jobs. |
@@ -667,6 +691,11 @@ impl<T: Config> Pallet<T> { | |||
disputes, | |||
parent_header, | |||
}; | |||
|
|||
// Write back to storage only keys that have been updated or deleted. | |||
for (para_id, candidates) in pending_availability_overlay.into_iter() { |
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.
What about using a Drop
instance like here?
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.
yes, good idea, I'll have to see how it should work in the case of this generic overlay.
{ | ||
// Construct a new overlay instance. | ||
pub fn new() -> Self { | ||
let data = P::populate(); |
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.
Given that we are doubling the memory footprint here, it would be really good if this improved performance by more than just a few percent.
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.
Right now it improves it by around 12% . I'd say it is not great, but that's what is currently possible. What kind of improvement are you expecting for the tradeoff to be worth it ?
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.
Good question. Depends on how tight we are on those axes. Time wise we are pretty good already. No idea about memory. Probably worth it.
But I just noticed something different: What this gains us, as far as I can see, is that it conflates two writes into one. At a quick glance it seems, that this should also easily be achievable by merging this mutate with the previous one. We are traversing them in order anyways. Instead of pushing to candidates_made_available, we could just enact immediately, if there were no holes (unavailable candidates in the loop before).
This should give us the same performance gain, but with half the memory consumption. @alindima what do you say?
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.
I'll think about it a bit. In the first mutate, we update the availability votes. But when doing the enactments, we may enact more candidates than the ones that have been just made available in this block.
If for example we have A B C backed. B and C are already available. A is just being made available now. So we have to enact all candidates (A B and C).
That's why I chose to iterate twice there. I'll think more about it to see if we can call mutate only once for each para
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
6a43427
to
03841e1
Compare
bot bench polkadot-pallet --runtime=westend --pallet=runtime_parachains::paras_inherent -v UPSTREAM_MERGE=n |
@sandreim https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5532841 was started for your command Comment |
…=westend --target_dir=polkadot --pallet=runtime_parachains::paras_inherent
@sandreim Command |
Extracted Benchbuilder enhancements used in #3644 . Might still require some work to fully support all scenarios when disputing elastic scaling parachains, but it should be useful in writing elastic scaling runtime tests. --------- Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
…3690) Extracted Benchbuilder enhancements used in paritytech#3644 . Might still require some work to fully support all scenarios when disputing elastic scaling parachains, but it should be useful in writing elastic scaling runtime tests. --------- Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Changes on top of #3479
Introduce a generic
StorageMapOverlay
and use it to efficiently mutatePendingAvailability
storage in memory. Early inprocess_inherent_data
we create this overlay and only write back what was changed before returning from the function.Runtime APIs must still access the storage directly.
This changeset doesn't attempt to solve any logic issues that have been identified in the review of #3479, but it does try to help with allowing to change values while iterating the in-memory map, currently at the cost of an extra clone.