Move heavy computation to a thread pool with a priority queue #6247

SimonSapin · 2024-11-07T17:31:07Z

These components can take non-trivial amounts of CPU time:

GraphQL parsing
GraphQL validation
Query planning
Schema introspection

In order to avoid blocking threads that execute asynchronous code, they are now run (in their respective Rust implementations) in a new pool of as many threads as CPU cores are available. Previously we used Tokio’s spawn_blocking for this purpose, but it is appears to be intended for blocking I/O and uses up to 512 threads so it isn’t a great fit for computation tasks.

Additionally, QueryPlannerPool is bypassed for the new planner since the new thread pool and queue fulfill the same role. This makes the new planner parallel by default (whereas the old pool defaults to size 1, making planning effectively sequential).

The first commit supersedes and closes #6122. The second (non-merge) commit supersedes and closes #6142.

The ageing priority algorithm is based on @garypen’s work in https://github.com/apollographql/ageing

Fixes ROUTER-827

Checklist

Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.

Exceptions

Note any exceptions here

Notes

It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this. ↩
Configuration is an important part of many changes. Where applicable please try to document configuration examples. ↩
Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions. ↩

@garypen

These components can take non-trivial amounts of CPU time: * GraphQL parsing * GraphQL validation * Query planning * Schema introspection In order to avoid blocking threads that execute asynchronous code, they are now run (in their respective Rust implementations) in a new pool of as many threads as CPU cores are available. Previously we used Tokio’s [`spawn_blocking`] for this purpose, but it is appears to be intended for blocking I/O and uses up to 512 threads so it isn’t a great fit for computation tasks. [`spawn_blocking`]: https://docs.rs/tokio/latest/tokio/task/fn.spawn_blocking.html This PR supersedes and closes #6122 The ageing priority algorithm is based on @garypen’s work in https://github.com/apollographql/ageing

svc-apollo-docs · 2024-11-07T17:31:12Z

✅ Docs Preview Ready

No new or changed pages found.

router-perf · 2024-11-07T17:31:41Z

CI performance tests

SimonSapin · 2024-11-08T13:00:39Z

apollo-router/src/compute_job.rs

+/// We generate backpressure in tower `poll_ready` when reaching this many queued items
+// TODO: what’s a good number? should it be configurable?
+const QUEUE_SOFT_CAPACITY: usize = 100;
+
+// TODO: should this be configurable?
+fn thread_pool_size() -> NonZeroUsize {
+    std::thread::available_parallelism().expect("available_parallelism() failed")
+}


Some open questions here

My opinion is that if we make something configurable just because we don’t know what a good value would be, most users won’t know either.

I agree. Better to try and think of a good default and only make it configurable (if ever) later.

Should it be available - 1 so we keep 1 core free to handle traffic? Or is it fine to rely on the OS scheduler to still let traffic go through the router?

My thinking with the initial PR was to rely on the OS scheduler, but minus one might be ok too. The downside is that for example minus one out of 2 available cores has a much bigger impact that minus one out of 32 cores

Here's my proposal:

size: max(1, available - (ceiling(available/8)))

WORKINGS:
AVAILABLE: 1 POOL SIZE: 1
AVAILABLE: 2 POOL SIZE: 1
AVAILABLE: 3 POOL SIZE: 2
AVAILABLE: 4 POOL SIZE: 3
AVAILABLE: 5 POOL SIZE: 4
...
AVAILABLE: 8 POOL SIZE: 7
AVAILABLE: 9 POOL SIZE: 7
...
AVAILABLE: 16 POOL SIZE: 14
AVAILABLE: 17 POOL SIZE: 14
...
AVAILABLE: 32 POOL SIZE: 28

Tweaks on the basic approach are welcome, but it seems to offer reasonable scaling for query planning. We can always refine it later.

Because each task in the pool (by default just one) takes queries to plan from a queue in sequence, it is a parallelism bottleneck even in "new" mode

SimonSapin · 2024-11-08T13:13:29Z

apollo-router/src/compute_job.rs

+    meter_provider()
+        .meter("apollo/router")
+        .u64_observable_gauge("apollo.router.compute_jobs.queued")
+        .with_description(
+            "Number of computation jobs (parsing, planning, …) waiting to be scheduled",
+        )
+        .with_callback(move |m| m.observe(queue().queued_count() as u64, &[]))
+        .init()


This is a new gauge metric. apollo.router.query_planning.queued will only exist when the legacy planner is used. It is somewhat replaced by the new metric but not exactly since the new queue also contains parsing+validation jobs and introspection jobs.

This new metric should be documented.

I’ve added a short description in docs/source/reference/router/telemetry/instrumentation/standard-instruments.mdx. Are there other places to add it to?

I think that's the correct location.

garypen

This looks great in general. Maybe we can find some time over the next couple of days to discuss how we figure out good default values for queue/pool sizes.

garypen · 2024-11-11T08:51:18Z

apollo-router/src/compute_job.rs

+/// We generate backpressure in tower `poll_ready` when reaching this many queued items
+// TODO: what’s a good number? should it be configurable?
+const QUEUE_SOFT_CAPACITY: usize = 100;
+
+// TODO: should this be configurable?
+fn thread_pool_size() -> NonZeroUsize {
+    std::thread::available_parallelism().expect("available_parallelism() failed")
+}


I agree. Better to try and think of a good default and only make it configurable (if ever) later.

garypen · 2024-11-11T09:07:02Z

apollo-router/src/compute_job.rs

+
+/// We generate backpressure in tower `poll_ready` when reaching this many queued items
+// TODO: what’s a good number? should it be configurable?
+const QUEUE_SOFT_CAPACITY: usize = 100;


I don't think it should be configurable initially. I think this queue represents the tradeoff between memory and losing time here vs being re-scheduled onto a different router, if one is available. If we are rejected from the queue here, then we know at least we have to spend the time/work to move the job elsewhere.

That's hard to quantify, but it's likely in the order of milliseconds. Perhaps we can workshop up a rough calculation based on this thinking?

garypen · 2024-11-11T09:22:41Z

apollo-router/src/compute_job.rs

+    meter_provider()
+        .meter("apollo/router")
+        .u64_observable_gauge("apollo.router.compute_jobs.queued")
+        .with_description(
+            "Number of computation jobs (parsing, planning, …) waiting to be scheduled",
+        )
+        .with_callback(move |m| m.observe(queue().queued_count() as u64, &[]))
+        .init()


This new metric should be documented.

apollo-router/src/compute_job.rs

apollo-router/src/ageing_priority_queue.rs

garypen

The remaining decision about QUEUE_SOFT_CAPACITY is problematic. It might be best understood as a function of the thread_pool_size() for now.

Proposal: 20 * thread_pool_size() as a starting point.

I'm approving this as is, but I'd like to know what you think of my sizing suggestions.

garypen · 2024-11-19T12:01:07Z

apollo-router/src/compute_job.rs

+/// We generate backpressure in tower `poll_ready` when reaching this many queued items
+// TODO: what’s a good number? should it be configurable?
+const QUEUE_SOFT_CAPACITY: usize = 100;
+
+// TODO: should this be configurable?
+fn thread_pool_size() -> NonZeroUsize {
+    std::thread::available_parallelism().expect("available_parallelism() failed")
+}


Here's my proposal:

size: max(1, available - (ceiling(available/8)))

WORKINGS:
AVAILABLE: 1 POOL SIZE: 1
AVAILABLE: 2 POOL SIZE: 1
AVAILABLE: 3 POOL SIZE: 2
AVAILABLE: 4 POOL SIZE: 3
AVAILABLE: 5 POOL SIZE: 4
...
AVAILABLE: 8 POOL SIZE: 7
AVAILABLE: 9 POOL SIZE: 7
...
AVAILABLE: 16 POOL SIZE: 14
AVAILABLE: 17 POOL SIZE: 14
...
AVAILABLE: 32 POOL SIZE: 28

Tweaks on the basic approach are welcome, but it seems to offer reasonable scaling for query planning. We can always refine it later.

garypen · 2024-11-19T12:01:52Z

apollo-router/src/compute_job.rs

+    meter_provider()
+        .meter("apollo/router")
+        .u64_observable_gauge("apollo.router.compute_jobs.queued")
+        .with_description(
+            "Number of computation jobs (parsing, planning, …) waiting to be scheduled",
+        )
+        .with_callback(move |m| m.observe(queue().queued_count() as u64, &[]))
+        .init()


I think that's the correct location.

garypen · 2024-11-19T12:03:05Z

apollo-router/src/compute_job.rs

+
+    #[tokio::test]
+    async fn test_parallelism() {
+        if thread_pool_size().get() < 2 {


SimonSapin · 2024-11-19T13:37:33Z

I’ve updated the changelog and enabled auto-merge

…#6247)" This reverts commit 8e1928c.

Merge remote-tracking branch 'origin/dev' into simon/compute-jobs

43b353f

SimonSapin commented Nov 8, 2024

View reviewed changes

Make BridgeQueryPlannerPool pass-through for new QP

d2557cf

Because each task in the pool (by default just one) takes queries to plan from a queue in sequence, it is a parallelism bottleneck even in "new" mode

SimonSapin commented Nov 8, 2024

View reviewed changes

SimonSapin marked this pull request as ready for review November 8, 2024 13:16

SimonSapin requested review from a team as code owners November 8, 2024 13:16

garypen reviewed Nov 11, 2024

View reviewed changes

SimonSapin added 3 commits November 18, 2024 10:03

Merge branch 'dev' into simon/compute-jobs

7517ca1

Unit tests

32bf712

Metrics docs

9bdd6b2

garypen approved these changes Nov 19, 2024

View reviewed changes

SimonSapin added 2 commits November 19, 2024 14:29

Tweak thread pool size and queue capacity

6267a1f

Changelog

679c1de

SimonSapin enabled auto-merge (squash) November 19, 2024 13:37

SimonSapin merged commit 8e1928c into dev Nov 19, 2024
14 checks passed

SimonSapin deleted the simon/compute-jobs branch November 19, 2024 13:52

abernix mentioned this pull request Nov 26, 2024

prep release: v1.58.0 #6331

Merged

BrynCooke added a commit that referenced this pull request Nov 26, 2024

Revert "Move heavy computation to a thread pool with a priority queue (…

339c3f2

…#6247)" This reverts commit 8e1928c.

BrynCooke mentioned this pull request Nov 26, 2024

test: Revert "Move heavy computation to a thread pool with a priority queue" #6334

Closed

SimonSapin mentioned this pull request Nov 29, 2024

explore using a priority queue with the threadpool #6186

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move heavy computation to a thread pool with a priority queue #6247

Move heavy computation to a thread pool with a priority queue #6247

SimonSapin commented Nov 7, 2024 •

edited by garypen

Loading

svc-apollo-docs commented Nov 7, 2024 •

edited

Loading

router-perf bot commented Nov 7, 2024

SimonSapin Nov 8, 2024

garypen Nov 11, 2024

goto-bus-stop Nov 18, 2024

SimonSapin Nov 19, 2024

garypen Nov 19, 2024

SimonSapin Nov 8, 2024

garypen Nov 11, 2024

SimonSapin Nov 18, 2024

garypen Nov 19, 2024

garypen left a comment

garypen Nov 11, 2024

garypen Nov 11, 2024

garypen Nov 11, 2024

garypen left a comment

garypen Nov 19, 2024

garypen Nov 19, 2024

garypen Nov 19, 2024

SimonSapin commented Nov 19, 2024

Move heavy computation to a thread pool with a priority queue #6247

Move heavy computation to a thread pool with a priority queue #6247

Conversation

SimonSapin commented Nov 7, 2024 • edited by garypen Loading

Footnotes

svc-apollo-docs commented Nov 7, 2024 • edited Loading

✅ Docs Preview Ready

router-perf bot commented Nov 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garypen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garypen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimonSapin commented Nov 19, 2024

SimonSapin commented Nov 7, 2024 •

edited by garypen

Loading

svc-apollo-docs commented Nov 7, 2024 •

edited

Loading