page_service: rewrite batching to work without a timeout, pipeline in protocol handler instead

[problame]

Problem

The timeout-based batching adds latency to unbatchable workloads.

We can choose a short batching timeout (e.g. 10us) but that requires high-resolution timers, which tokio doesn't have.
I thoroughly explored options to use OS timers (see this abandoned PR).
In short, it's not an attractive option because any timer implementation adds non-trivial overheads.

Solution

The insight is that, in the steady state of a batchable workload, the time we spend in get_vectored will be hundreds of microseconds anyway.

If we prepare the next batch concurrently to get_vectored, we will have a sizeable batch ready once get_vectored of the current batch is done and do not need an explicit timeout.

This can be reasonably described as pipelining of the protocol handler.

Implementation

We model the sub-protocol handler for pagestream requests (handle_pagrequests) as three futures that form a pipeline:

1. Reading: read messages from pgb
2. Batching: fill the current batch
3. Execution: take the current batch, execute it using get_vectored, and send the response.

The Reading and Batching stage are conencted through an mpsc channel.

The Batching and Execution stage use a quirky construct to coordinate:

1. An Arc<std::sync::Mutex<Option<Box<BatchedFeMessage>>>> that represents the current batch.
2. A watch around it to notify Execution about new data.
3. a Notify to notify Batch about data consumed.
4. Inside the watch, a Mutex<BatchedFeMessage>

This construct allows the Execution stage to at any time, steal the current batch from Batching, using lock().unwrap().take().

Changes

• Refactor handle_pagerequests
  • separate functions for
    • reading one protocol message; produces a BatchedFeMessage with just one page request in it
    • batching; tried to merge an incoming BatchedFeMessage into an existing BatchedFeMessage; returns None on success and returns back the incoming message in case merging isn't possible
    • execution of a batched message
  • unify the timeline handle acquisition & request span construction; it now happen in the function that reads the protocol message
• Implement serial and pipelined model
  • serial: what we had before any of the batching changes
    • read one protocol message
    • execute protocol messages
  • pipelined: the design described above
    • optionality for execution of the pipeline: either via concurrent futures vs tokio tasks
• Pageserver config
  • remove batching timeout field
  • add ability to configure max batch size (required for the rollout, cf https://github.com/neondatabase/cloud/issues/20620 )
  • ability to configure execution mode
• Tests
  • remove batch_timeout parametrization
  • rename test_getpage_merge_smoke to test_throughput
    • add parametrization to test different max batch sizes and execution moes
  • rename test_timer_precision to test_latency
  • rename the teast case file to test_page_service_batching.py
  • better descriptions of what the tests actually do

On the holding The TimelineHandle in the pending batch

While batching, we hold the TimelineHandle in the pending batch.
Therefore, the timeline will not finish shutting down while we're batching.

This is not a problem in practice because the concurrently ongoing get_vectored call will fail quickly with an error indicating that the timeline is shutting down.
This results in the Execution stage returning a QueryError::Shutdown, which causes the pipeline / entire page service connection to shut down.
This drops all references to the Arc<Mutex<Option<Box<BatchedFeMessage>>>> object, thereby dropping the contained TimelineHandles.

• => fixes page_service: figure out correctness of holding the TimelineHandle in the pending batch #9850

Performance

Local run of the benchmarks, results in this empty commit in the PR branch.

Use commands like this to compare a particular metric in different configurations.

git show cbe18393d390961fc3dcf61287fdae2dcddcdf6b  | grep -E '(None|tasks)' | grep '.batching_factor'

Key take-aways:

• concurrent-futures delivers higher batching_factor than tasks
  • tail latency impact unknown, cf page_service: measure tail latency impact in batchable workload #9837
• concurrent-futures has lower CPU usage
• throughput (time) is better with concurrent-futures except in the case of unbatchable workload with max batch size 1; in that case, tasks is 6% better but consume more CPU time for the same work
• un-batchable latency impact is much better than what we saw with timeout-based batching
  • mean: 117us => 120us (concurrent-futures) => 127us (task)
  • tail latencies:
    • concurrent-futures consistently slightly better than tasks, difference neglegible
    • p99.9 and lower are approximately identical in all configurations
    • p99.99 of serial is significantly better: 429us vs pipelined configurations are ~550us

Refs

• epic: Epic: get page throughput improvements #9376
• this sub-task: pageserver: batch get page requests and serve them with one vectored get #9377
• the abandoned attempt to improve batching timeout resolution: page_service: add benchmark for batching #9820
• closes page_service: figure out correctness of holding the TimelineHandle in the pending batch #9850
• fixes page_service: batching needless waits for unbatchable requests #9835

[github-actions]

5535 tests run: 5305 passed, 4 failed, 226 skipped (full report)

---

Failures on Postgres 17

• test_detach_while_attaching: release-x86-64

Failures on Postgres 16

• test_pageserver_reconnect: release-arm64
• test_pageserver_restarts_under_worload: release-arm64

Failures on Postgres 14

• test_pageserver_reconnect: release-arm64

# Run all failed tests locally:
scripts/pytest -vv -n $(nproc) -k "test_pageserver_reconnect[release-pg14] or test_pageserver_reconnect[release-pg16] or test_pageserver_restarts_under_worload[release-pg16] or test_detach_while_attaching[release-pg17]"

Flaky tests (5)

Postgres 17

• test_sharding_split_failures[failure13]: debug-x86-64

Postgres 15

• test_pageserver_restarts_under_worload: release-arm64, release-x86-64
• test_pull_timeline[True]: release-arm64

Postgres 14

• test_pageserver_restarts_under_worload: release-arm64

Test coverage report is not available

_{The comment gets automatically updated with the latest test results
41ddc67 at 2024-11-26T14:34:44.587Z :recycle:}

[VladLazar]

Went through all of it minus benchmark. Flushing comments.

Logic looks good to me, but have some questions about control flow (see #9851 (comment)).

[VladLazar]

Could we use MAX_GET_VECTORED_KEYS instead?

[problame]

Hm, initially I was thinking to revert back to None before merging - it's Some() right now so I get CI coverage.

Let's keep this conversation open and decide later.

[VladLazar]

I'm not sure I see this.

Let's say that protocol_pipelining_mode=PageServiceProtocolPipeliningMode::Tasks and the call to pagesteam_handle_batched_message.

handle_pagerequests_pipelined returns an error, but read_message_task and batcher_task are still running. They'll run until a message is received an realise that downstream is dead. For the ConcurrentFutures case, I think it works fine.

[VladLazar]

For the ConcurrentFutures case, I think it works fine.

Actually, I'm not sure about that. join! polls the futures from a lambda so a failure in one of the futures doesn't bail the function (I think). This is problematic for cases where we want to kill the pagestream connection on the server side (e.g. wrong shard).

[problame]

Hmm yes, correct. Will address this tomorrow.

[VladLazar]

nit: does this deserve a comment mentioning that the channel has "memory" (i.e. if the change happens while we are processing things below, it still gets detected on the next call)?

[problame]

Can you check the new abstracted-out sync primitive and see whether this comment still applies? #9851 (comment)

[VladLazar]

watch::channel already wraps the value in a read/write lock. Can we skip this Mutex?

[problame]

The watch::Receiver doesn't allow mutable access, so, yes, this Mutex is necessary in the current constellation.

I asked in #rust for a better primitive: https://neondb.slack.com/archives/C0277TKAJCA/p1732277778048099

[problame]

implemented in 9bf2618 and adopted in a23abb2

[VladLazar]

Can we enable this in a few test unit/python to get some rudimentary continuous test coverage?

[problame]

Can we enable this in a few test unit/python to get some rudimentary continuous test coverage?

I don't get the question. If the default is Some here, so, we have the entire test suite as coverage.

[VladLazar]

I assumed you won't merge it with pipelining enabled. If that's the case, I am suggesting to parametrize some existing python tests to use pipelining (we'd also have to tweak the compute config).

[problame]

The timeline_handles may be None in this case.

[conradludgate]

What's the distinction between these two cases? My understanding is that we don't need to notify the sender during HasData