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

Fix algorithmic complexity of on-demand scheduler with regards to number of cores. #3190

Merged
merged 35 commits into from
Mar 20, 2024

Conversation

eskimor
Copy link
Member

@eskimor eskimor commented Feb 2, 2024

We witnessed really poor performance on Rococo, where we ended up with 50 on-demand cores. This was due to the fact that for each core the full queue was processed. With this change full queue processing will happen way less often (most of the time complexity is O(1) or O(log(n))) and if it happens then only for one core (in expectation).

Also spot price is now updated before each order to ensure economic back pressure.

TODO:

  • Implement
  • Basic tests
  • Add more tests (see todos)
  • Run benchmark to confirm better performance, first results suggest > 100x faster.
  • Write migrations
  • Bump scale-info version and remove patch in Cargo.toml
  • Write PR docs: on-demand performance improved, more on-demand cores are now non problematic anymore. If need by also the max queue size can be increased again. (Maybe not to 10k)

Optional: Performance can be improved even more, if we called pop_assignment_for_core(), before calling report_processed (Avoid needless affinity drops). The effect gets smaller the larger the claim queue and I would only go for it, if it does not add complexity to the scheduler.

@ordian
Copy link
Member

ordian commented Feb 2, 2024

New profile looks good 🚀
Screenshot 2024-02-02 at 16 50 38

Copy link
Member Author

@eskimor eskimor left a comment

Choose a reason for hiding this comment

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

Nice work, thanks @antonva !

antonva added a commit that referenced this pull request Mar 13, 2024
2.11.0  Implements TypeInfo for BinaryHeap (#200).
which is needed for #3190
@antonva antonva mentioned this pull request Mar 13, 2024
24 tasks
@command-bot
Copy link

command-bot bot commented Mar 18, 2024

@antonva Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=rococo --target_dir=polkadot --pallet=runtime_parachains::assigner_on_demand has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5559236 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5559236/artifacts/download.

@eskimor eskimor marked this pull request as ready for review March 18, 2024 14:16
@antonva
Copy link
Contributor

antonva commented Mar 18, 2024

bot bench polkadot-pallet --runtime=westend --pallet=runtime_parachains::assigner_on_demand

@command-bot
Copy link

command-bot bot commented Mar 18, 2024

@antonva https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5561742 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=westend --target_dir=polkadot --pallet=runtime_parachains::assigner_on_demand. Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 1-1a2e10ad-a1ef-48dc-a0d4-abb401a78939 to cancel this command or bot cancel to cancel all commands in this pull request.

…=westend --target_dir=polkadot --pallet=runtime_parachains::assigner_on_demand
@command-bot
Copy link

command-bot bot commented Mar 18, 2024

@antonva Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=westend --target_dir=polkadot --pallet=runtime_parachains::assigner_on_demand has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5561742 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5561742/artifacts/download.

@ordian ordian self-requested a review March 18, 2024 15:36
Copy link
Member

@ordian ordian left a comment

Choose a reason for hiding this comment

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

Approving modulo nits, but I'm not deeply familiar with the underlying logic of the on_demand assigner

/// The actual queue is implemented via multiple priority queues. One for each core, for entries
/// which currently have a core affinity and one free queue, with entries without any affinity yet.
///
/// The design aims to have most queue accessess be O(1) or O(log(N)). Absolute worst case is O(N).
Copy link
Member

Choose a reason for hiding this comment

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

a note on complexity: when you have a map like AffinityEntries from core index to BinaryHeap, the read and write to it would still be O(len(BinaryHeap)). Having BinaryHeap instead of a Vec only saves on in-memory operations, but I assume these will be dominated by the actual db writes/reads + allocation + encoding/decoding + hashing unless you have a lot of in-memory ops for one read/write

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean, because the data still needs to be fetched? Yes indeed, I considered this, but decided to not make it part of the complexity analysis, because the fetching of data is unavoidable and not really part of the algorithm. I was also hoping that the batch data fetching is fast. In any case, we are also usually fetching less data than before. I was tempted to optimize not accessing the free list, in those cases where we don't need data from there, by keeping track of its status, but decided the complexity is not worth it.

Comment on lines +718 to +722
#[cfg(not(test))]
debug_assert_ne!(
affinity, None,
"Decreased affinity for a para that has not been served on a core?"
);
Copy link
Member

Choose a reason for hiding this comment

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

first time seeing debug assert for non test environment. is the intention to run this assert on testnets and benchmarks only?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess it breaks some edge case tests?

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable 3/3
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5581597

@antonva antonva added this pull request to the merge queue Mar 20, 2024
@antonva antonva added the T8-polkadot This PR/Issue is related to/affects the Polkadot network. label Mar 20, 2024
Merged via the queue into master with commit b74353d Mar 20, 2024
132 of 137 checks passed
@antonva antonva deleted the rk-on-demand-perf-proper-fix branch March 20, 2024 14:29
dharjeezy pushed a commit to dharjeezy/polkadot-sdk that referenced this pull request Mar 24, 2024
…ber of cores. (paritytech#3190)

We witnessed really poor performance on Rococo, where we ended up with
50 on-demand cores. This was due to the fact that for each core the full
queue was processed. With this change full queue processing will happen
way less often (most of the time complexity is O(1) or O(log(n))) and if
it happens then only for one core (in expectation).

Also spot price is now updated before each order to ensure economic back
pressure.


TODO:

- [x] Implement
- [x] Basic tests
- [x] Add more tests (see todos)
- [x] Run benchmark to confirm better performance, first results suggest
> 100x faster.
- [x] Write migrations
- [x] Bump scale-info version and remove patch in Cargo.toml
- [x] Write PR docs: on-demand performance improved, more on-demand
cores are now non problematic anymore. If need by also the max queue
size can be increased again. (Maybe not to 10k)

Optional: Performance can be improved even more, if we called
`pop_assignment_for_core()`, before calling `report_processed` (Avoid
needless affinity drops). The effect gets smaller the larger the claim
queue and I would only go for it, if it does not add complexity to the
scheduler.

---------

Co-authored-by: eskimor <[email protected]>
Co-authored-by: antonva <[email protected]>
Co-authored-by: command-bot <>
Co-authored-by: Anton Vilhelm Ásgeirsson <[email protected]>
Co-authored-by: ordian <[email protected]>
@Morganamilo Morganamilo mentioned this pull request Apr 4, 2024
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T8-polkadot This PR/Issue is related to/affects the Polkadot network.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Large OnDemandQueue queue processing leads to up to 10s block import times
4 participants